Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
On Wed, Aug 27, 2014 at 11:09:08PM +0200, Bjørn Mork wrote: > Greg Kroah-Hartman 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:v10DBd8808sv*sd*bc0Csc03iFE* pch_udc > alias pci:v10DBd801Dsv*sd*bc0Csc03iFE* pch_udc > alias pci:v8086d8808sv*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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
Greg Kroah-Hartman 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:v10DBd8808sv*sd*bc0Csc03iFE* pch_udc alias pci:v10DBd801Dsv*sd*bc0Csc03iFE* pch_udc alias pci:v8086d8808sv*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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
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
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
Hello Greg
On Wed, Aug 27, 2014 at 9:25 PM, Greg Kroah-Hartman
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
Hello Greg
On Wed, Aug 27, 2014 at 9:25 PM, Greg Kroah-Hartman
gre...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
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
gre...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
Greg Kroah-Hartman gre...@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:v10DBd8808sv*sd*bc0Csc03iFE* pch_udc alias pci:v10DBd801Dsv*sd*bc0Csc03iFE* pch_udc alias pci:v8086d8808sv*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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: gadget: net2280: Remove pci_class from PCI_TABLE
On Wed, Aug 27, 2014 at 11:09:08PM +0200, Bjørn Mork wrote: Greg Kroah-Hartman gre...@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:v10DBd8808sv*sd*bc0Csc03iFE* pch_udc alias pci:v10DBd801Dsv*sd*bc0Csc03iFE* pch_udc alias pci:v8086d8808sv*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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
