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

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

[Ricardo Ribalda Delgado]

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.

This means that some implementations modprobe will fail to load the
driver.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/usb/gadget/udc/net2280.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index f4eac11..542ab89 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -3770,16 +3770,12 @@ static void net2280_shutdown(struct pci_dev *pdev)
 /*-------------------------------------------------------------------------*/
 
 static const struct pci_device_id pci_ids[] = { {
-	.class =	((PCI_CLASS_SERIAL_USB << 8) | 0xfe),
-	.class_mask =	~0,
 	.vendor =	PCI_VENDOR_ID_PLX_LEGACY,
 	.device =	0x2280,
 	.subvendor =	PCI_ANY_ID,
 	.subdevice =	PCI_ANY_ID,
 	.driver_data =	PLX_LEGACY | PLX_2280,
 	}, {
-	.class =	((PCI_CLASS_SERIAL_USB << 8) | 0xfe),
-	.class_mask =	~0,
 	.vendor =	PCI_VENDOR_ID_PLX_LEGACY,
 	.device =	0x2282,
 	.subvendor =	PCI_ANY_ID,
@@ -3787,8 +3783,6 @@ static const struct pci_device_id pci_ids[] = { {
 	.driver_data =	PLX_LEGACY,
 	},
 	{
-	.class =	((PCI_CLASS_SERIAL_USB << 8) | 0xfe),
-	.class_mask =	~0,
 	.vendor =	PCI_VENDOR_ID_PLX,
 	.device =	0x3380,
 	.subvendor =	PCI_ANY_ID,
@@ -3796,8 +3790,6 @@ static const struct pci_device_id pci_ids[] = { {
 	.driver_data =	PLX_SUPERSPEED,
 	 },
 	{
-	.class =	((PCI_CLASS_SERIAL_USB << 8) | 0xfe),
-	.class_mask =	~0,
 	.vendor =	PCI_VENDOR_ID_PLX,
 	.device =	0x3382,
 	.subvendor =	PCI_ANY_ID,
-- 
2.1.0

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

[Greg Kroah-Hartman]

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.

> 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?

And I mean incorrect, as you are changing the values here from being
very specific, to being much broader.

thanks,

greg k-h

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

[Ricardo Ribalda Delgado]

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

>
> 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.


> thanks,

Thank you :)
>
> greg k-h



-- 
Ricardo Ribalda

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

[Greg Kroah-Hartman]

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

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

[Ricardo Ribalda Delgado]

Hello Greg

>>
>> 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.

On the other thread ( https://lkml.org/lkml/2014/8/27/242 ) we have
agreed about fixing this thing on pci-sysfs.c .

Still I think that there is no good reason to add the pci interface to
the pci_table on this driver. Therefore I consider that this patch is
still valid.

What do you think. This patch is NACK?


Thanks!

>
> thanks,
>
> greg k-h



-- 
Ricardo Ribalda

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

[Bjørn Mork]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Wed, Aug 27, 2014 at 09:39:43PM +0200, Ricardo Ribalda Delgado wrote:
>> 
>> return sprintf(buf, "pci:v%08Xd%08Xsv%08Xsd%08Xbc%02Xsc%02Xi%02x\n",

That final 'x' does look like a typo, doesn't it?  We are otherwise
consistently using upper-case hex digits for field values and lower case
letter for field names.  But it looks like it has been like that since
the beginning, so it might be difficult to fix...

> 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.

Now I don't really know much about the history here, but the format of
module aliases, using wildcards, seem to suggest a string match to me.
Do you really mean that these strings should be parsed into field names
+ values before matching?  If that was the intention then surely we
would have exported the fields one-by-one as separate sysfs attributes?
Ref the "one value per file" policy.

>> 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.

well, just looking at a few common PCI devices on my PCs I wonder if the
reason this hasn't been a problem is because there are _very_ few PCI
programming interfaces using anything by 0-9 digits.  One?  Looking at the
modules built by Debian I can only find one udc module matching on any
hex value:

 bjorn@nemi:~$  grep pci: /lib/modules/3.16-trunk-amd64/modules.alias|egrep "i[A-F]"
 alias pci:v000010DBd00008808sv*sd*bc0Csc03iFE* pch_udc
 alias pci:v000010DBd0000801Dsv*sd*bc0Csc03iFE* pch_udc
 alias pci:v00008086d00008808sv*sd*bc0Csc03iFE* pch_udc

This makes me wonder if this is exclusively a problem for PCI UDCs,
which tend to be pretty rare devices?


Bjørn

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

[Greg Kroah-Hartman]

On Wed, Aug 27, 2014 at 11:03:00PM +0200, Ricardo Ribalda Delgado wrote:
> Hello Greg
> 
> >>
> >> 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.
> 
> On the other thread ( https://lkml.org/lkml/2014/8/27/242 ) we have
> agreed about fixing this thing on pci-sysfs.c .
> 
> Still I think that there is no good reason to add the pci interface to
> the pci_table on this driver. Therefore I consider that this patch is
> still valid.
> 
> What do you think. This patch is NACK?

Yeah, I don't think this patch is needed as it properly sets the class
of the device to be matched against, so it should not be necessary at
all.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Wed, Aug 27, 2014 at 11:09:08PM +0200, Bjørn Mork wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > On Wed, Aug 27, 2014 at 09:39:43PM +0200, Ricardo Ribalda Delgado wrote:
> >> 
> >> return sprintf(buf, "pci:v%08Xd%08Xsv%08Xsd%08Xbc%02Xsc%02Xi%02x\n",
> 
> That final 'x' does look like a typo, doesn't it?  We are otherwise
> consistently using upper-case hex digits for field values and lower case
> letter for field names.  But it looks like it has been like that since
> the beginning, so it might be difficult to fix...

Yes, it should be fixed, sorry, my later email said that, no one has hit
it in 9+ years, pretty impressive.

> > 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.
> 
> Now I don't really know much about the history here, but the format of
> module aliases, using wildcards, seem to suggest a string match to me.
> Do you really mean that these strings should be parsed into field names
> + values before matching?  If that was the intention then surely we
> would have exported the fields one-by-one as separate sysfs attributes?
> Ref the "one value per file" policy.

No, this is a bit field, so you can't do a string compare.  kmod should
know how do handle this, it does so for other types of "class" fields in
module device ids.

And no, we didn't export these as a set of files, it's one unique value
that you can use to match up a device to a driver.

> >> 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.
> 
> well, just looking at a few common PCI devices on my PCs I wonder if the
> reason this hasn't been a problem is because there are _very_ few PCI
> programming interfaces using anything by 0-9 digits.  One?  Looking at the
> modules built by Debian I can only find one udc module matching on any
> hex value:
> 
>  bjorn@nemi:~$  grep pci: /lib/modules/3.16-trunk-amd64/modules.alias|egrep "i[A-F]"
>  alias pci:v000010DBd00008808sv*sd*bc0Csc03iFE* pch_udc
>  alias pci:v000010DBd0000801Dsv*sd*bc0Csc03iFE* pch_udc
>  alias pci:v00008086d00008808sv*sd*bc0Csc03iFE* pch_udc
> 
> This makes me wonder if this is exclusively a problem for PCI UDCs,
> which tend to be pretty rare devices?

Yes, this is a rare thing, as the age of this line proves :)

thanks,

greg k-h