Bug 201609

I found same error on Redhat bugtracker:
https://bugzilla.redhat.com/show_bug.cgi?id=1443678

Some people says that kernel 4.9.x is not affected

Hi,

I've stumbled upon this very same issue, and have created a github repo with a fix for the actual issue (see git commit's description for bug details).

https://github.com/pruiz/aic94xx-hotfix
https://github.com/pruiz/aic94xx-hotfix/commit/01898ea7dff8eb52f43773ea3b59ab41ef2482c8

Hope it helps someone.

Best Regards

Please consider to do a root-cause analysis, to check which patch introduced this bug and to send the patch upstream according to the instructions in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst.

On Sat, 2019-01-26 at 03:13 +0000, bugzilla-daemon@bugzilla.kernel.org
wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=201609
> 
> Pablo Ruiz García (pablo.ruiz@gmail.com) changed:
> 
>            What    |Removed                     |Added
> -------------------------------------------------------------------
> ---------
>                  CC|                            |pablo.ruiz@gmail.com
> 
> --- Comment #2 from Pablo Ruiz García (pablo.ruiz@gmail.com) ---
> Hi,
> 
> I've stumbled upon this very same issue, and have created a github
> repo with a
> fix for the actual issue (see git commit's description for bug
> details).
> 
> https://github.com/pruiz/aic94xx-hotfix
> https://github.com/pruiz/aic94xx-hotfix/commit/01898ea7dff8eb52f43773
> ea3b59ab41ef2482c8
> 
> Hope it helps someone.

I think the correct fix is to rename the ai94xx revision file (say to
aic94x_revision). However I've got to say that commit

commit 702ed3be1b1bf4dea05954168321741c0910c645
Author: Emil Velikov <emil.velikov@collabora.com>
Date:   Mon Nov 21 16:24:49 2016 -0600

    PCI: Create revision file in sysfs
  
Was a blatently silly thing to do without trying to namespace the added
file (say calling it something less generic like pci_revision).  I
assume we're lucky and nothing in userspace depends on the 'revision'
sysfs file of the aic driver, but technically I should request
reversion of this patch on the grounds that it's broken our ABI (since
that's what exported sysfs files are).

James

The other five files ({subsystem_,}{vendor,device} and class) do not have a prefix, so the more reasonable thing was it not add one for the revision file.

That said, I did spend a lot of time going through the Documentation looking for guidelines, rules or restrictions that should be applied and could not find any.
The PCI subsystem maintainer did not suggest adding a prefix either, so I would not use "blatently silly" here.

On a more practical side - the aic94xx driver custom revision file solves the exact same problem my patch does. Admittedly, aic94xx lacks the leading "0x" for the hexadecimal number provided.

My personal inclination is that we'd want to check if existing userspace requires the leading 0x and if so to what extend:
 - cosmetic - remove the aic94xx code, update userspace
 - unused - remove the aic94xx code
 - major impact - the aic94xx driver removes the generic revision file, before setting it's own

Does that sound reasonable, am I missing something?

I stand corrected:
The aic94xx revision file exposes a string (which look like hexadecimal number) based on the revision number.

Regardless, the ideas proposed earlier are still applicable.

I don't know what the sysfs namespace rules are (if there are any).  It doesn't sound ideal for the PCI core to prefix all the files in a PCI device directory with "pci_" or similar, since we already know by the path (/sys/devices/pci0000:00, /sys/bus/pci/devices, etc) that they are PCI-related).

On Wed, 2019-01-30 at 14:21 +0000, bugzilla-daemon@bugzilla.kernel.org
wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=201609
> 
> Bjorn Helgaas (bhelgaas@google.com) changed:
> 
>            What    |Removed                     |Added
> -------------------------------------------------------------------
> ---------
>                  CC|                            |bhelgaas@google.com
> 
> --- Comment #7 from Bjorn Helgaas (bhelgaas@google.com) ---
> I don't know what the sysfs namespace rules are (if there are
> any).  It doesn't sound ideal for the PCI core to prefix all the
> files in a PCI device directory with "pci_" or similar, since we
> already know by the path (/sys/devices/pci0000:00,
> /sys/bus/pci/devices, etc) that they are PCI-related).

In the early days, we all simply grabbed files in the directory.  This
resulted in a complete lack of namespacing for everything.  Arguably
this was the really silly thing to do but we stuck with it.  What it
means is that now we have to be very careful about adding files because
things like this can easily happen.  In particular, if you add a
generic name like "revision" there's a really good chance some driver
already has it.

THere's nothing we can do about the prior lack of namespacing, but if
you don't want to check every driver in the kernel, using a namespace
prefix is a good way of avoiding clashes like this.  You can still add
a non-namespaced file, it's just you're on the hook to check every
driver to prevent cockups like this.

The key point is that if the file you clashed with is in use by
userspace, we can't rename it because it's an ABI, so now the conflict
is unresolvable.  I don't think this applies in the aic driver, but
it's the reason why you need to be very careful about adding files.

James

On Wed, 2019-01-30 at 11:43 +0000, bugzilla-daemon@bugzilla.kernel.org
wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=201609
> 
> Emil Velikov (emil.l.velikov@gmail.com) changed:
> 
>            What    |Removed                     |Added
> -------------------------------------------------------------------
> ---------
>                  CC|                            |emil.l.velikov@gmail
> .com
> 
> --- Comment #5 from Emil Velikov (emil.l.velikov@gmail.com) ---
> The other five files ({subsystem_,}{vendor,device} and class) do not
> have a prefix, so the more reasonable thing was it not add one for
> the revision file.

No, it wasn't.  This is the problem.  In the early days we all grabbed
un-namespaced names in this directory.  If you add another un-
namespaced file, a clash is likely to happen which has serious
consequences.  If you want to add a generically named file, you have to
check all the drivers before doing it.  If you clash with a file that's
in use by userspace, your new addition gets reverted because we can't
rename an in-use file because it's an exported ABI.

> That said, I did spend a lot of time going through the Documentation
> looking for guidelines, rules or restrictions that should be applied
> and could not find any. The PCI subsystem maintainer did not suggest
> adding a prefix either, so I would not use "blatently silly" here.

Either you prefix the addition or you check every driver ... neither
happened in this case.

> On a more practical side - the aic94xx driver custom revision file
> solves the exact same problem my patch does. Admittedly, aic94xx
> lacks the leading "0x" for the hexadecimal number provided.
> 
> My personal inclination is that we'd want to check if existing
> userspace
> requires the leading 0x and if so to what extend:
>  - cosmetic - remove the aic94xx code, update userspace
>  - unused - remove the aic94xx code
>  - major impact - the aic94xx driver removes the generic revision
> file, before setting it's own
> 
> Does that sound reasonable, am I missing something?

I think we're safe and the aic file isn't used from userspace, so just
renaming the aic revision file should work.

James

(In reply to James.Bottomley from comment #9)
> On Wed, 2019-01-30 at 11:43 +0000, bugzilla-daemon@bugzilla.kernel.org
> wrote:
> > https://bugzilla.kernel.org/show_bug.cgi?id=201609
> > 
> > Emil Velikov (emil.l.velikov@gmail.com) changed:
> > 
> >            What    |Removed                     |Added
> > -------------------------------------------------------------------
> > ---------
> >                  CC|                            |emil.l.velikov@gmail
> > .com
> > 
> > --- Comment #5 from Emil Velikov (emil.l.velikov@gmail.com) ---
> > The other five files ({subsystem_,}{vendor,device} and class) do not
> > have a prefix, so the more reasonable thing was it not add one for
> > the revision file.
> 
> No, it wasn't.  This is the problem.  In the early days we all grabbed
> un-namespaced names in this directory.  If you add another un-
> namespaced file, a clash is likely to happen which has serious
> consequences.  If you want to add a generically named file, you have to
> check all the drivers before doing it.  If you clash with a file that's
> in use by userspace, your new addition gets reverted because we can't
> rename an in-use file because it's an exported ABI.
> 
I see thank you. Can we get this and the (check every driver) documented please? Otherwise we'll end up in the same situation more often than needed.

The usual regex in MAINTAINERS is unlikely to catch such patches, so it will be hard for experienced developers/maintainers to catch these easily :-\

Fwiw many platforms depend on the revision entry that I've introduced. From desktops (using the open-source graphics stack) to servers depending on libvirt.

> > That said, I did spend a lot of time going through the Documentation
> > looking for guidelines, rules or restrictions that should be applied
> > and could not find any. The PCI subsystem maintainer did not suggest
> > adding a prefix either, so I would not use "blatently silly" here.
> 
> Either you prefix the addition or you check every driver ... neither
> happened in this case.
> 
Ack. Going through the kernel:

$ git grep "\<DEVICE_ATTR\>.*\<revision\>"

drivers/base/soc.c:static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
drivers/scsi/aic94xx/aic94xx_init.c:static DEVICE_ATTR(revision, S_IRUGO, asd_show_dev_rev, NULL);
drivers/video/fbdev/gbefb.c:static DEVICE_ATTR(revision, S_IRUGO, gbefb_show_rev, NULL);


First is safe as is the last one (a platform_device). So only aic needs updating.

> > On a more practical side - the aic94xx driver custom revision file
> > solves the exact same problem my patch does. Admittedly, aic94xx
> > lacks the leading "0x" for the hexadecimal number provided.
> > 
> > My personal inclination is that we'd want to check if existing
> > userspace
> > requires the leading 0x and if so to what extend:
> >  - cosmetic - remove the aic94xx code, update userspace
> >  - unused - remove the aic94xx code
> >  - major impact - the aic94xx driver removes the generic revision
> > file, before setting it's own
> > 
> > Does that sound reasonable, am I missing something?
> 
> I think we're safe and the aic file isn't used from userspace, so just
> renaming the aic revision file should work.
> 
Ack. Are you going to write a patch or shall I?
Any name recommendations - aic94xx_revision, aic_revision, other?

Thanks
Emil