Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Aug 27, 2014 at 09:39:43PM +0200, Ricardo Ribalda Delgado wrote:
> Hello Greg
> 
> 
> 
> 
> 
> On Wed, Aug 27, 2014 at 9:25 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Aug 27, 2014 at 03:00:29PM +0200, Ricardo Ribalda Delgado wrote:
> >> Defining the vendor and the product id should be enough to discriminate
> >> the device.
> >>
> >> The reason for this patch is that there is a missmatch betweed the
> >> modalias showed by sysfs and the modalias generated by file2alias.
> >>
> >> One expects the programming interface in uppercase and the other
> >> generates it in lowercase.
> >
> > I don't understand, what is wrong here?  Who does it in uppercase and
> > who in lower?  And does it matter?  It's just a numeric value that
> > should not be used as a string compare.
> >
> 
> In pci-sysfs: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-sysfs.c#n175
> 
> static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
>     char *buf)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
> 
> return sprintf(buf, "pci:v%08Xd%08Xsv%08Xsd%08Xbc%02Xsc%02Xi%02x\n",
>       pci_dev->vendor, pci_dev->device,
>       pci_dev->subsystem_vendor, pci_dev->subsystem_device,
>       (u8)(pci_dev->class >> 16), (u8)(pci_dev->class >> 8),
>       (u8)(pci_dev->class));
> }
> 
> 
> In file2alias: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/mod/file2alias.c
> 
> #define ADD(str, sep, cond, field)                              \
> do {                                                            \
>         strcat(str, sep);                                       \
>         if (cond)                                               \
>                 sprintf(str + strlen(str),                      \
>                         sizeof(field) == 1 ? "%02X" :           \
>                         sizeof(field) == 2 ? "%04X" :           \
>                         sizeof(field) == 4 ? "%08X" : "",       \
>                         field);                                 \
>         else                                                    \
>                 sprintf(str + strlen(str), "*");                \
> } while(0)
> 
> 
> ADD(alias, "bc", baseclass_mask == 0xFF, baseclass);
> ADD(alias, "sc", subclass_mask == 0xFF, subclass);
> ADD(alias, "i", interface_mask == 0xFF, interface);
> 
> 
> 
> >> This means that some implementations modprobe will fail to load the
> >> driver.
> >
> > What implementations fail to work?  Shouldn't we fix the root of the
> > problem and not just patch up all drivers to display incorrect data?
> 
> At least the implementation of kmod in yocproject does a case sensitive match.
> 
> 
> I have already sent a patch to fix what I consider the root of the problem
> 
> https://lkml.org/lkml/2014/8/27/242

No, the root cause of the problem is a userspace tool looking at a hex
value as a string and not a number.  It doesn't matter if we print it in
upper or lower case, it's a digit, not a string.  Do the numeric
compare, not a string compare.

> > And I mean incorrect, as you are changing the values here from being
> > very specific, to being much broader.
> >
> 
> Not many drivers define the pci interface and there is no other driver
> that has the same vendor  and product id. Therefore I see no hurt in
> adding both patches, one to make the driver broader, and another to
> fix pci-sysfs.
> 
> Also, the change on pci-sysfs might affect more stuff and therefore
> take longer to be applied.

As we have been printing the value to userspace in this way for well
over a decade now, and nothing has changed, I say it's a userspace bug
that you should fix instead.  Don't work around broken user programs in
the kernel by changing something that has been stable for 10+ years.

Ok, sorry, not 10+ years, the commit was written May of 2005, so 9
years.

Fix your module loading code please.

thanks,

greg k-h