Hi Denis, Regarding your below query
- Can you tell me why this new addition uses vid/pid with a 0x prefix while the rest of the table does not? It seems weird & inconsistent. Actually in case of pci device vendor and device id read using libudev API is returning as "0x8086" and "0x7560" strings respectively. So I have added the same values in vendor_list table. In case of usb device the vendor and device strings returned by same libudev API does not have "0x" prefix. I don’t have any idea why it is so. If you insist I can remove this prefix 0x from vendor_list table and change the implementation of check_pci_device() function to handle comparison of vendor and model strings with vid, pid form vendor_list. Regards Antara -----Original Message----- From: Denis Kenzior [mailto:[email protected]] Sent: Thursday, September 26, 2019 7:55 AM To: Borwankar, Antara <[email protected]>; [email protected] Subject: Re: [PATCH] udev: Adding PCIe as a subsystem in udev Hi Antara, On 9/25/19 4:55 AM, Antara Borwankar wrote: > Adding support for enumerating PCIe types of modems in ofono > --- > plugins/udevng.c | 195 > +++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 174 insertions(+), 21 deletions(-) > > diff --git a/plugins/udevng.c b/plugins/udevng.c index > 1b54b4a..bebbc2d 100644 > --- a/plugins/udevng.c > +++ b/plugins/udevng.c > @@ -41,6 +41,7 @@ > enum modem_type { > MODEM_TYPE_USB, > MODEM_TYPE_SERIAL, > + MODEM_TYPE_PCIE, > }; > > struct modem_info { > @@ -1229,26 +1230,43 @@ static gboolean setup_xmm7xxx(struct modem_info > *modem) > info->interface, info->number, info->label, > info->sysattr, info->subsystem); > > - if (g_strcmp0(modem->model,"095a") == 0) { > - if (g_strcmp0(info->subsystem, "tty") == 0) { > - if (g_strcmp0(info->number, "00") == 0) > - mdm = info->devnode; > - } else if (g_strcmp0(info->subsystem, "net") == 0) { > - if (g_strcmp0(info->number, "06") == 0) > - net = info->devnode; > - if (g_strcmp0(info->number, "08") == 0) > - net2 = info->devnode; > - if (g_strcmp0(info->number, "0a") == 0) > - net3 = info->devnode; > + if (g_strcmp0(info->subsystem, "pci") == 0) { > + if ((g_strcmp0(modem->vendor,"0x8086") == 0) && > + (g_strcmp0(modem->model,"0x7560") == > 0)) { > + mdm = "/dev/iat"; > + net = "inm0"; > + net2 = "inm1"; > + net3 = "inm2"; > + ofono_modem_set_string(modem->modem, > + "CtrlPath", > "/PCIE/IOSM/CTRL/1"); > + ofono_modem_set_string(modem->modem, "DataPath", > + "/PCIE/IOSM/IPS/"); > } > - } else { > - if (g_strcmp0(info->subsystem, "tty") == 0) { > - if (g_strcmp0(info->number, "02") == 0) > - mdm = info->devnode; > - } else if (g_strcmp0(info->subsystem, "net") == 0) { > - if (g_strcmp0(info->number, "00") == 0) > - net = info->devnode; > + } else { /* For USB */ > + if (g_strcmp0(modem->model,"095a") == 0) { > + if (g_strcmp0(info->subsystem, "tty") == 0) { > + if (g_strcmp0(info->number, "00") == 0) > + mdm = info->devnode; > + } else if (g_strcmp0(info->subsystem, "net") == > 0) { > + if (g_strcmp0(info->number, "06") == 0) > + net = info->devnode; > + if (g_strcmp0(info->number, "08") == 0) > + net2 = info->devnode; > + if (g_strcmp0(info->number, "0a") == 0) > + net3 = info->devnode; > + } > + } else { > + if (g_strcmp0(info->subsystem, "tty") == 0) { > + if (g_strcmp0(info->number, "02") == 0) > + mdm = info->devnode; > + } else if (g_strcmp0(info->subsystem, "net") == > 0) { > + if (g_strcmp0(info->number, "00") == 0) > + net = info->devnode; > + } > } > + > + ofono_modem_set_string(modem->modem, "CtrlPath", > "/USBCDC/0"); > + ofono_modem_set_string(modem->modem, "DataPath", > "/USBHS/NCM/"); Lines > 80 characters, please see doc/coding-style.txt > } > } > > @@ -1266,9 +1284,6 @@ static gboolean setup_xmm7xxx(struct modem_info *modem) > if (net3) > ofono_modem_set_string(modem->modem, "NetworkInterface3", net3); > > - ofono_modem_set_string(modem->modem, "CtrlPath", "/USBCDC/0"); > - ofono_modem_set_string(modem->modem, "DataPath", "/USBHS/NCM/"); > - > return TRUE; > } > > @@ -1437,6 +1452,7 @@ static void destroy_modem(gpointer data) > > switch (modem->type) { > case MODEM_TYPE_USB: > + case MODEM_TYPE_PCIE: > for (list = modem->devices; list; list = list->next) { > struct device_info *info = list->data; > > @@ -1467,6 +1483,7 @@ static gboolean check_remove(gpointer key, > gpointer value, gpointer user_data) > > switch (modem->type) { > case MODEM_TYPE_USB: > + case MODEM_TYPE_PCIE: > for (list = modem->devices; list; list = list->next) { > struct device_info *info = list->data; > > @@ -1597,6 +1614,91 @@ static void add_serial_device(struct udev_device *dev) > modem->serial = info; > } > > +static void add_pci_device(const char *syspath, const char *devname, > + const char *driver, const char *vendor, > + const char *model, struct udev_device *device) { > + const char *devpath, *devnode, *interface, *number; > + const char *label, *sysattr, *subsystem; > + struct modem_info *modem; > + struct device_info *info; > + struct udev_device *parent; > + > + devpath = udev_device_get_syspath(device); > + > + if (devpath == NULL) > + return; > + > + devnode = udev_device_get_devnode(device); > + > + if (devnode == NULL) { > + devnode = udev_device_get_property_value(device, "INTERFACE"); > + DBG("devnode = %s", devnode); > + } > + > + modem = g_hash_table_lookup(modem_list, syspath); > + > + if (modem == NULL) { > + modem = g_try_new0(struct modem_info, 1); > + > + if (modem == NULL) > + return; > + > + modem->type = MODEM_TYPE_PCIE; > + modem->syspath = g_strdup(syspath); > + modem->devname = g_strdup(devname); > + modem->driver = g_strdup(driver); > + modem->vendor = g_strdup(vendor); > + modem->model = g_strdup(model); > + modem->sysattr = get_sysattr(driver); > + > + g_hash_table_replace(modem_list, modem->syspath, modem); > + } > + > + interface = udev_device_get_property_value(device, "INTERFACE"); > + > + /* > + * If environment variable is not set, get value from attributes > + * (or parent's ones) > + */ > + number = udev_device_get_sysattr_value(device, "bInterfaceNumber"); > + > + if (number == NULL) { > + parent = udev_device_get_parent(device); > + number = udev_device_get_sysattr_value(parent, > "bInterfaceNumber"); > + } > + > + label = udev_device_get_property_value(device, "OFONO_LABEL"); > + > + if (!label) > + label = udev_device_get_property_value(device, "OFONO_LABEL"); > + > + subsystem = udev_device_get_subsystem(device); > + > + if (modem->sysattr != NULL) > + sysattr = udev_device_get_sysattr_value(device, modem->sysattr); > + else > + sysattr = NULL; > + > + DBG("%s (%s) %s [%s] ==> %s %s", devnode, driver, > + interface, number, label, sysattr); > + info = g_try_new0(struct device_info, 1); > + > + if (info == NULL) > + return; > + > + info->devpath = g_strdup(devpath); > + info->devnode = g_strdup(devnode); > + info->interface = g_strdup(interface); > + info->number = g_strdup(number); > + info->label = g_strdup(label); > + info->sysattr = g_strdup(sysattr); > + info->subsystem = g_strdup(subsystem); > + > + modem->devices = g_slist_insert_sorted(modem->devices, info, > + compare_device); This seems to be an almost verbatim copy-paste of add_device. Can you not use that instead with some modifications? Or factor out the common operations into a separate function? > +} > + > static void add_device(const char *syspath, const char *devname, > const char *driver, const char *vendor, > const char *model, struct udev_device *device) @@ > -1625,8 > +1727,10 @@ static void add_device(const char *syspath, const char *devname, > return; > > modem = g_hash_table_lookup(modem_list, syspath); > + > if (modem == NULL) { > modem = g_try_new0(struct modem_info, 1); > + > if (modem == NULL) > return; > These changes seem to be superfluous and unrelated to this patch? > @@ -1772,6 +1876,7 @@ static struct { > { "telit", "cdc_acm", "1bc7", "0036" }, > { "xmm7xxx", "cdc_acm", "8087" }, > { "xmm7xxx", "cdc_ncm", "8087" }, > + { "xmm7xxx", "imc_ipc", "0x8086", "0x7560"}, Can you tell me why this new addition uses vid/pid with a 0x prefix while the rest of the table does not? It seems weird & inconsistent. > { } > }; > > @@ -1858,6 +1963,51 @@ static void check_usb_device(struct udev_device > *device) > add_device(syspath, devname, driver, vendor, model, device); > } > > +static void check_pci_device(struct udev_device *device) { > + const char *syspath, *devname, *driver; > + const char *vendor = NULL, *model = NULL, *drv = NULL; > + unsigned int i; > + > + syspath = udev_device_get_syspath(device); > + > + if (syspath == NULL) > + return; > + > + devname = udev_device_get_devnode(device); > + vendor = udev_device_get_sysattr_value(device, "vendor"); > + model = udev_device_get_sysattr_value(device, "device"); > + driver = udev_device_get_property_value(device, "OFONO_DRIVER"); > + drv = udev_device_get_property_value(device, "DRIVER"); > + DBG("%s [%s:%s]", drv, vendor, model); > + > + if (vendor == NULL || model == NULL) > + return; So should you be adding drv == NULL check above as well? > + > + for (i = 0; vendor_list[i].driver; i++) { > + if ((drv != NULL) && (g_str_equal(vendor_list[i].drv, drv) == > +TRUE)) That way you can avoid looping here and simplify your logic considerably > + DBG("vendor_list[%d].drv = %s", i, vendor_list[i].drv); Did you intend to leave this DBG in here? > + else > + continue; > + > + if (vendor_list[i].vid) { > + if (!g_str_equal(vendor_list[i].vid, vendor)) > + continue; > + } > + > + if (vendor_list[i].pid) { > + if (!g_str_equal(vendor_list[i].pid, model)) > + continue; > + } > + > + driver = vendor_list[i].driver; > + } > + > + if (driver == NULL) > + return; > + > + add_pci_device(syspath, devname, driver, vendor, model, device); } > static void check_device(struct udev_device *device) > { > const char *bus; > @@ -1872,6 +2022,8 @@ static void check_device(struct udev_device *device) > if ((g_str_equal(bus, "usb") == TRUE) || > (g_str_equal(bus, "usbmisc") == TRUE)) > check_usb_device(device); > + else if (g_str_equal(bus, "pci") == TRUE) > + check_pci_device(device); > else > add_serial_device(device); > > @@ -1932,6 +2084,7 @@ static void enumerate_devices(struct udev *context) > udev_enumerate_add_match_subsystem(enumerate, "usbmisc"); > udev_enumerate_add_match_subsystem(enumerate, "net"); > udev_enumerate_add_match_subsystem(enumerate, "hsi"); > + udev_enumerate_add_match_subsystem(enumerate, "pci"); > > udev_enumerate_scan_devices(enumerate); > > Regards, -Denis _______________________________________________ ofono mailing list [email protected] https://lists.ofono.org/mailman/listinfo/ofono
