Re: [PATCH v4 1/6] plugins/udevng: Add support for PCIe MBIM modems * Parses through the sysfs tree and detects MBIM control nodes, net and AT nodes

Hi Muhammad,

On 5/4/25 2:19 PM, Muhammad Asif wrote:
> ---
>   plugins/udevng.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
> 

Some general comments:

oFono still prefers that lines are limited to 80 characters, so please 
restructure accordingly.  Also, checkpatch still flags some other issues:

0001-plugins-udevng-Add-support-for-PCIe-MBIM-modems-Pars.patch:8: WARNING: 
Missing commit description - Add an appropriate one
0001-plugins-udevng-Add-support-for-PCIe-MBIM-modems-Pars.patch:70: WARNING: 
line length of 105 exceeds 100 columns
0001-plugins-udevng-Add-support-for-PCIe-MBIM-modems-Pars.patch:72: WARNING: Too 
many leading tabs - consider code refactoring
0001-plugins-udevng-Add-support-for-PCIe-MBIM-modems-Pars.patch:74: WARNING: Too 
many leading tabs - consider code refactoring

> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index b8df66de..09c916db 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1176,7 +1176,13 @@ static gboolean setup_mbim(struct modem_info *modem)
>   {
>   	const char *ctl = NULL, *net = NULL, *atcmd = NULL;
>   	GSList *list;
> +	const char *sub_subsystem = NULL, *type = NULL;
> +	char path[512], wwan_path[1024], sub_path[2048];
>   	char descriptors[PATH_MAX];
> +	struct udev *new_udev;
> +	struct udev_device *wwan_device, *sub_device;
> +	struct dirent *dir = NULL, *subdir = NULL;
> +	DIR *d = NULL, *sd = NULL;
>   
>   	DBG("%s [%s:%s]", modem->syspath, modem->vendor, modem->model);
>   

I see that you added "mbim" to a list of pci drivers in patch 4.  This patch and 
patch 4 should be combined into a single patch.  Can you provide a log from 
'ofonod -nd'?  Especially after patch 4, oFono should be detecting all nodes 
related to this device.  Or, hmm...

Perhaps there needs to be some additional logic in check_wwan_device() as well? 
Right now it only detects MHI devices.

> @@ -1196,6 +1202,56 @@ static gboolean setup_mbim(struct modem_info *modem)
>   		else if (g_strcmp0(subsystem, "tty") == 0) {
>   			if (g_strcmp0(info->number, "02") == 0)
>   				atcmd = info->devnode;
> +		} else if (g_strcmp0(subsystem, "pci") == 0) {
> +			sprintf(path, "%s/wwan", udev_device_get_syspath(info->udev_device));
> +
> +			d = opendir(path);
> +			if (!d)
> +				return FALSE;

Hmm, so you are detecting some node (I assume root) of the device, then open 
coding custom detection logic based on the wwan subsystem provided sysfs links. 
What if the WWAN subsystem isn't enabled in the kernel?  Can udevng be made to 
work without these links?

At a high level, if WWAN subsystem is enabled, then udevng is redundant.  There 
needs to be a dedicated wwan subsystem detection plugin, however this is a 
missing piece in oFono at the moment.  Marcel, didn't you have some beginnings 
of this?

> +
> +			new_udev = udev_new();
> +
> +			while ((dir = readdir(d))) {
> +				if (g_strcmp0(dir->d_name, ".") == 0 ||
> +					g_strcmp0(dir->d_name, "..") == 0)
> +					continue;
> +
> +				sprintf(wwan_path, "%s/%s", path, dir->d_name);
> +
> +				wwan_device = udev_device_new_from_syspath(new_udev,
> +									wwan_path);
> +				net = udev_device_get_sysname(wwan_device);
> +
> +				// Parse all the subdirectories now

nit: We use C comments, not C++

> +				sd = opendir(wwan_path);
> +				while ((subdir = readdir(sd))) {
> +					if (subdir->d_type != DT_DIR)
> +						continue;
> +
> +					if (g_strcmp0(subdir->d_name, ".") == 0 ||
> +						g_strcmp0(subdir->d_name, "..") == 0)
> +						continue;
> +
> +					sprintf(sub_path, "%s/%s", wwan_path, subdir->d_name);
> +
> +					sub_device = udev_device_new_from_syspath(new_udev,
> +										sub_path);
> +					sub_subsystem = udev_device_get_subsystem(sub_device);
> +
> +					if (g_strcmp0(sub_subsystem, "wwan") == 0) {
> +						type = udev_device_get_sysattr_value(sub_device, "type");
> +
> +						if (g_strcmp0(type, "MBIM") == 0)
> +							ctl = udev_device_get_devnode(sub_device);
> +						else if (g_strcmp0(type, "AT") == 0)
> +							atcmd = udev_device_get_devnode(sub_device);
> +					}
> +				}
> +				closedir(sd);
> +				break;
> +			}
> +			closedir(d);
> +			udev_unref(new_udev);
>   		}
>   	}
>   

Regards,
-Denis