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

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

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