pci: add to global list before sysfs?

pci: add to global list before sysfs?

[Pierre Ossman]

Hi Greg,

I have a PCI hotplug problem I was hoping you could provide some
insight into.

I want the PCI layer to make sure that all subfunctions of a newly
added card are visible via pci_get_device() before any of them are
binded to drivers. I.e. I want pci_bus_add_devices() to first make a
pass and add the devices to the global list, then a second pass for
device_add().

The reason I want this is because I have at least two chips where I
need to know which other subfunctions are present on the card in order
to correctly initalise the subfunction I'm currently probing. Right now
that fails miserably as the driver does not see the other devices in
its probe function.

Is it possible to do this or will things break left and right if I
add them to the global list before I register them with the driver core?

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: pci: add to global list before sysfs?

[Greg KH]

On Fri, Apr 04, 2008 at 07:13:50PM +0200, Pierre Ossman wrote:
> Hi Greg,
> 
> I have a PCI hotplug problem I was hoping you could provide some
> insight into.
> 
> I want the PCI layer to make sure that all subfunctions of a newly
> added card are visible via pci_get_device() before any of them are
> binded to drivers. I.e. I want pci_bus_add_devices() to first make a
> pass and add the devices to the global list, then a second pass for
> device_add().

What "global list"?  You mean the one that I just deleted with my
patchset that is in -mm and going to be in 2.6.26?  :)

> The reason I want this is because I have at least two chips where I
> need to know which other subfunctions are present on the card in order
> to correctly initalise the subfunction I'm currently probing. Right now
> that fails miserably as the driver does not see the other devices in
> its probe function.

How will changing the addition of the device to the list affect this
behavior?  Do you want to walk the list in your driver's probe function,
looking for the "subfunction" that is not yet added to the list?

> Is it possible to do this or will things break left and right if I
> add them to the global list before I register them with the driver core?

As I mention above, the "global list" is now gone, so it's going to be a
bit harder to do this.  We now only keep one list of all PCI devices,
and that is with the driver core mechanisms as the duplicate list was
just sitting there doing nothing for the past 4 years.

So is the problem that your driver need to bind to multiple pci devices
at the same time in order to work properly?

Or is this just really broken hardware?

confused,

greg k-h

Re: pci: add to global list before sysfs?

[Pierre Ossman]

On Fri, 4 Apr 2008 14:01:32 -0700
Greg KH <gregkh@suse.de> wrote:

> 
> What "global list"?  You mean the one that I just deleted with my
> patchset that is in -mm and going to be in 2.6.26?  :)
> 

*gnn*

> 
> How will changing the addition of the device to the list affect this
> behavior?  Do you want to walk the list in your driver's probe function,
> looking for the "subfunction" that is not yet added to the list?
> 

Yes. Things work fine when the driver is loaded after the entire PCI
bus has been scanned, but not when new cards are hotplugged with the
driver already present. I'd like the behaviour to be the same in both
cases.

> > Is it possible to do this or will things break left and right if I
> > add them to the global list before I register them with the driver core?
> 
> As I mention above, the "global list" is now gone, so it's going to be a
> bit harder to do this.  We now only keep one list of all PCI devices,
> and that is with the driver core mechanisms as the duplicate list was
> just sitting there doing nothing for the past 4 years.
> 

Talk about bad timing...

> So is the problem that your driver need to bind to multiple pci devices
> at the same time in order to work properly?
> 

More or less. Primarily it needs to know which other subfunctions are
there, but in one of the two cases it also needs to fiddle some PCI
config space registers.

> Or is this just really broken hardware?
> 

It is. Deliberately broken in order to work with a certain other broken
operating system.

They have two interfaces for the same piece of hardware, and you need
to make sure you only use one of them or things go south very quickly.
To make things worse, you do not know which of the two will be
available for any given card. Hence the need to scan the bus.

In one of the cases we currently try to disable the secondary
interface, which is done by PCI config space modifications on
subfunction 0.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: pci: add to global list before sysfs?

[Greg KH]

On Sat, Apr 05, 2008 at 11:57:27AM +0200, Pierre Ossman wrote:
> On Fri, 4 Apr 2008 14:01:32 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
> > 
> > What "global list"?  You mean the one that I just deleted with my
> > patchset that is in -mm and going to be in 2.6.26?  :)
> > 
> 
> *gnn*

Well the "global list" was not being used for anything, and it was being
created at the same time the driver core list was, so unless you wanted
to change the way that all pci devices were probed/created, it would not
have worked out.

> > How will changing the addition of the device to the list affect this
> > behavior?  Do you want to walk the list in your driver's probe function,
> > looking for the "subfunction" that is not yet added to the list?
> > 
> 
> Yes. Things work fine when the driver is loaded after the entire PCI
> bus has been scanned, but not when new cards are hotplugged with the
> driver already present. I'd like the behaviour to be the same in both
> cases.

Why not just create a thread in your driver, or timer, and sleep until
you see the other devices?  If after a certian time you never do, then
bail out.

That should work properly for you for both cases (startup and loading
later.)

thanks,

greg k-h

Re: pci: add to global list before sysfs?

[Pierre Ossman]

On Sat, 5 Apr 2008 08:53:23 -0700
Greg KH <gregkh@suse.de> wrote:

> On Sat, Apr 05, 2008 at 11:57:27AM +0200, Pierre Ossman wrote:
> > 
> > Yes. Things work fine when the driver is loaded after the entire PCI
> > bus has been scanned, but not when new cards are hotplugged with the
> > driver already present. I'd like the behaviour to be the same in both
> > cases.
> 
> Why not just create a thread in your driver, or timer, and sleep until
> you see the other devices?  If after a certian time you never do, then
> bail out.
> 
> That should work properly for you for both cases (startup and loading
> later.)
> 

I'm not sure how that could be done in a clean way. Right now I have
the following:

                sd_dev = NULL;
                while ((sd_dev = pci_get_device(PCI_VENDOR_ID_JMICRON,
                        PCI_DEVICE_ID_JMICRON_JMB38X_SD, sd_dev)) != NULL) {
                        if ((PCI_SLOT(chip->pdev->devfn) ==
                                PCI_SLOT(sd_dev->devfn)) &&
                                (chip->pdev->bus == sd_dev->bus))
                                break;
                }

                if (sd_dev) {
                        pci_dev_put(sd_dev);
                        dev_info(&chip->pdev->dev, "Refusing to bind to "
                                "secondary interface.\n");
                        return -ENODEV;
                }

If I do this threaded, I'd have to return success to the driver core
and then act like a possum later. Doable, but not very clean. It could
be solved if it was possible to sleep in probe functions, but from what
I can tell that will stop the probing of subsequent devices.

Also, the timeout would introduce an inherent race. Which means either
putting a high timeout to make the race insignificant, or a low timeout
so users don't have to wait for their device to actually become usable.

Is it possible to write some routines that can walk the PCI device tree
without having any pci_dev structures to hang on to? To solve my basic
problem I just need to know how the tree looks like, not actually poke
any of the other devices.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: pci: add to global list before sysfs?

[Greg KH]

On Sat, Apr 05, 2008 at 06:55:01PM +0200, Pierre Ossman wrote:
> On Sat, 5 Apr 2008 08:53:23 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
> > On Sat, Apr 05, 2008 at 11:57:27AM +0200, Pierre Ossman wrote:
> > > 
> > > Yes. Things work fine when the driver is loaded after the entire PCI
> > > bus has been scanned, but not when new cards are hotplugged with the
> > > driver already present. I'd like the behaviour to be the same in both
> > > cases.
> > 
> > Why not just create a thread in your driver, or timer, and sleep until
> > you see the other devices?  If after a certian time you never do, then
> > bail out.
> > 
> > That should work properly for you for both cases (startup and loading
> > later.)
> > 
> 
> I'm not sure how that could be done in a clean way. Right now I have
> the following:
> 
>                 sd_dev = NULL;
>                 while ((sd_dev = pci_get_device(PCI_VENDOR_ID_JMICRON,
>                         PCI_DEVICE_ID_JMICRON_JMB38X_SD, sd_dev)) != NULL) {
>                         if ((PCI_SLOT(chip->pdev->devfn) ==
>                                 PCI_SLOT(sd_dev->devfn)) &&
>                                 (chip->pdev->bus == sd_dev->bus))
>                                 break;
>                 }
> 
>                 if (sd_dev) {
>                         pci_dev_put(sd_dev);
>                         dev_info(&chip->pdev->dev, "Refusing to bind to "
>                                 "secondary interface.\n");
>                         return -ENODEV;
>                 }
> 
> If I do this threaded, I'd have to return success to the driver core
> and then act like a possum later.

Yes.

> Doable, but not very clean.

Hey, take it up with the hardware designers compensating for an
"unclean" operating system :)

> It could be solved if it was possible to sleep in probe functions, but
> from what I can tell that will stop the probing of subsequent devices.

Yes, you can sleep, but it will stop the probe of subsequent devices.  I
did implement multiple threads for pci probing, but it ended up not
saving any time and causing more problems than it was worth.

> Also, the timeout would introduce an inherent race. Which means either
> putting a high timeout to make the race insignificant, or a low timeout
> so users don't have to wait for their device to actually become usable.
> 
> Is it possible to write some routines that can walk the PCI device tree
> without having any pci_dev structures to hang on to? To solve my basic
> problem I just need to know how the tree looks like, not actually poke
> any of the other devices.

No, not in a sane way as all arches walk the pci device tree for
discovery in many different ways.

I don't think that a time-out would be a big deal, it shouldn't be very
long (a second or two).  Also note that all distros would not have this
problem as they do pci discovery first, and then start loading all of
the modules in the initramfs after the pci tree has been properly
scanned.  So the very large majority of users would not have a problem.

Or are you doing this on an embedded system?

thanks,

greg k-h

Re: pci: add to global list before sysfs?

[Pierre Ossman]

On Sat, 5 Apr 2008 10:12:09 -0700
Greg KH <gregkh@suse.de> wrote:

> On Sat, Apr 05, 2008 at 06:55:01PM +0200, Pierre Ossman wrote:
> > If I do this threaded, I'd have to return success to the driver core
> > and then act like a possum later.
> 
> Yes.
> 
> > Doable, but not very clean.
> 
> Hey, take it up with the hardware designers compensating for an
> "unclean" operating system :)
> 

I have. And unfortunately that other operating system is just too big
of a market to ignore. :/

This is a cooperating vendor though, which is why I'm trying to put a
bit more extra effort into actually getting their devices fully
supported. Telling them to ditch Windows support is asking too much.

Anyway, a big part of my reluctance to go this route is that this chip
is just one of many the driver supports. So I'm not thrilled to do some
funky architecture for this device as it'll most certainly cause me
pain for the others...

> I don't think that a time-out would be a big deal, it shouldn't be very
> long (a second or two).  Also note that all distros would not have this
> problem as they do pci discovery first, and then start loading all of
> the modules in the initramfs after the pci tree has been properly
> scanned.  So the very large majority of users would not have a problem.
> 
> Or are you doing this on an embedded system?
> 

No, it's primarily desktop. But these chips are available as
ExpressCards, so I'm trying to get them to work when hot-plugged as
opposed to when permantently wired into the machine.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: pci: add to global list before sysfs?

[Greg KH]

On Sat, Apr 05, 2008 at 08:27:09PM +0200, Pierre Ossman wrote:
> On Sat, 5 Apr 2008 10:12:09 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
> > On Sat, Apr 05, 2008 at 06:55:01PM +0200, Pierre Ossman wrote:
> > > If I do this threaded, I'd have to return success to the driver core
> > > and then act like a possum later.
> > 
> > Yes.
> > 
> > > Doable, but not very clean.
> > 
> > Hey, take it up with the hardware designers compensating for an
> > "unclean" operating system :)
> > 
> 
> I have. And unfortunately that other operating system is just too big
> of a market to ignore. :/
> 
> This is a cooperating vendor though, which is why I'm trying to put a
> bit more extra effort into actually getting their devices fully
> supported. Telling them to ditch Windows support is asking too much.
> 
> Anyway, a big part of my reluctance to go this route is that this chip
> is just one of many the driver supports. So I'm not thrilled to do some
> funky architecture for this device as it'll most certainly cause me
> pain for the others...
> 
> > I don't think that a time-out would be a big deal, it shouldn't be very
> > long (a second or two).  Also note that all distros would not have this
> > problem as they do pci discovery first, and then start loading all of
> > the modules in the initramfs after the pci tree has been properly
> > scanned.  So the very large majority of users would not have a problem.
> > 
> > Or are you doing this on an embedded system?
> > 
> 
> No, it's primarily desktop. But these chips are available as
> ExpressCards, so I'm trying to get them to work when hot-plugged as
> opposed to when permantently wired into the machine.

Then I suggest a thread/timer, it shouldn't be that difficult.

thanks,

greg k-h

Re: pci: add to global list before sysfs?

[Pierre Ossman]

On Mon, 7 Apr 2008 20:41:26 -0700
Greg KH <gregkh@suse.de> wrote:

> On Sat, Apr 05, 2008 at 08:27:09PM +0200, Pierre Ossman wrote:
> > 
> > No, it's primarily desktop. But these chips are available as
> > ExpressCards, so I'm trying to get them to work when hot-plugged as
> > opposed to when permantently wired into the machine.
> 
> Then I suggest a thread/timer, it shouldn't be that difficult.
> 

I have another idea. Is it guaranteed that the PCI subsystem will add
devices in order? I.e. will 1f.0 be added before 1f.1, 1f.2 and so on?
If so, then I can restructure things and get it to work properly
without any timers.

Btw, is your @kroah address working? I have a kernel bugzilla entry I
was hoping to get your input on.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: pci: add to global list before sysfs?

[Greg KH]

On Sun, Apr 13, 2008 at 02:33:11PM +0200, Pierre Ossman wrote:
> On Mon, 7 Apr 2008 20:41:26 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
> > On Sat, Apr 05, 2008 at 08:27:09PM +0200, Pierre Ossman wrote:
> > > 
> > > No, it's primarily desktop. But these chips are available as
> > > ExpressCards, so I'm trying to get them to work when hot-plugged as
> > > opposed to when permantently wired into the machine.
> > 
> > Then I suggest a thread/timer, it shouldn't be that difficult.
> > 
> 
> I have another idea. Is it guaranteed that the PCI subsystem will add
> devices in order? I.e. will 1f.0 be added before 1f.1, 1f.2 and so on?
> If so, then I can restructure things and get it to work properly
> without any timers.

Yes, that should the be the order in which devices are always scanned.

> Btw, is your @kroah address working? I have a kernel bugzilla entry I
> was hoping to get your input on.

Yes, that address works very well...

thanks,

greg k-h

Re: pci: add to global list before sysfs?

[Benjamin Herrenschmidt]

On Sat, 2008-04-05 at 18:55 +0200, Pierre Ossman wrote:
> > Why not just create a thread in your driver, or timer, and sleep
> until
> > you see the other devices?  If after a certian time you never do,
> then
> > bail out.
> > 
> > That should work properly for you for both cases (startup and
> loading
> > later.)
> > 

You don't need a timer. You can just use the notifier mechanism I added
a while ago that should call you on device addition and removal on the
bus type you are interested in.

Ben.

Re: pci: add to global list before sysfs?

[Pierre Ossman]

On Mon, 14 Apr 2008 07:37:14 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> You don't need a timer. You can just use the notifier mechanism I added
> a while ago that should call you on device addition and removal on the
> bus type you are interested in.
> 

Another solution presented itself, but do you have some pointers on
where I get more information about the notifiers in case I need them
in the future?

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: pci: add to global list before sysfs?

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 10:35 +0200, Pierre Ossman wrote:
> On Mon, 14 Apr 2008 07:37:14 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > 
> > You don't need a timer. You can just use the notifier mechanism I added
> > a while ago that should call you on device addition and removal on the
> > bus type you are interested in.
> > 
> 
> Another solution presented itself, but do you have some pointers on
> where I get more information about the notifiers in case I need them
> in the future?

In devices.h :

Re: pci: add to global list before sysfs?

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 10:35 +0200, Pierre Ossman wrote:
> On Mon, 14 Apr 2008 07:37:14 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > 
> > You don't need a timer. You can just use the notifier mechanism I added
> > a while ago that should call you on device addition and removal on the
> > bus type you are interested in.
> > 
> 
> Another solution presented itself, but do you have some pointers on
> where I get more information about the notifiers in case I need them
> in the future?

In device.h:

/*
 * Bus notifiers: Get notified of addition/removal of devices
 * and binding/unbinding of drivers to devices.
 * In the long run, it should be a replacement for the platform
 * notify hooks.
 */
struct notifier_block;

extern int bus_register_notifier(struct bus_type *bus,
				 struct notifier_block *nb);
extern int bus_unregister_notifier(struct bus_type *bus,
				   struct notifier_block *nb);

/* All 4 notifers below get called with the target struct device *
 * as an argument. Note that those functions are likely to be called
 * with the device semaphore held in the core, so be careful.
 */
#define BUS_NOTIFY_ADD_DEVICE		0x00000001 /* device added */
#define BUS_NOTIFY_DEL_DEVICE		0x00000002 /* device removed */
#define BUS_NOTIFY_BOUND_DRIVER		0x00000003 /* driver bound to device */
#define BUS_NOTIFY_UNBIND_DRIVER	0x00000004 /* driver about to be
						      unbound */
Do you need more info ?

Cheers,
Ben.

Re: pci: add to global list before sysfs?

[Pierre Ossman]

On Mon, 14 Apr 2008 18:58:36 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> On Mon, 2008-04-14 at 10:35 +0200, Pierre Ossman wrote:
> > 
> > Another solution presented itself, but do you have some pointers on
> > where I get more information about the notifiers in case I need them
> > in the future?
> 
> In device.h:
> 

*snip*

> 						      unbound */
> Do you need more info ?
> 

That'll do fine. Thanks.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: pci: add to global list before sysfs?

[Benjamin Herrenschmidt]

On Fri, 2008-04-04 at 14:01 -0700, Greg KH wrote:
> As I mention above, the "global list" is now gone, so it's going to be
> a
> bit harder to do this.  We now only keep one list of all PCI devices,
> and that is with the driver core mechanisms as the duplicate list was
> just sitting there doing nothing for the past 4 years.
> 
> So is the problem that your driver need to bind to multiple pci
> devices
> at the same time in order to work properly?
> 
> Or is this just really broken hardware?

Haven't had a chance to look at your changes yet, but I have memories of
being pretty happy with having that 2 step mechanism in PCI where the
driver core would only see the devices after fixup.

The reason for that is powerpc, like many other archs, need to do all
sorts of fixups before devices get used by drivers. Resources need to be
fixed up, interrupts, etc...

A lot of that happens in pcibios_fixup_bus() and we did rely on having
no driver bound before that happens. The binding can only happen after
the arch calls pci_bus_add_devices().

Is that still the case ? If not, then there is some big potential
breakage here and we need to look closely at re-organizing the way we do
our fixups.

Cheers,
Ben.

Re: pci: add to global list before sysfs?

[Greg KH]

On Mon, Apr 14, 2008 at 07:35:38AM +1000, Benjamin Herrenschmidt wrote:
> 
> On Fri, 2008-04-04 at 14:01 -0700, Greg KH wrote:
> > As I mention above, the "global list" is now gone, so it's going to be
> > a
> > bit harder to do this.  We now only keep one list of all PCI devices,
> > and that is with the driver core mechanisms as the duplicate list was
> > just sitting there doing nothing for the past 4 years.
> > 
> > So is the problem that your driver need to bind to multiple pci
> > devices
> > at the same time in order to work properly?
> > 
> > Or is this just really broken hardware?
> 
> Haven't had a chance to look at your changes yet, but I have memories of
> being pretty happy with having that 2 step mechanism in PCI where the
> driver core would only see the devices after fixup.

No, that is left alone, I just dropped the duplicate device list that
was linked within the PCI devices themselves.  The only user of it was
the "reorder the PCI devices" code for some Dell machines, and that was
just doing it to be safe (it was also reordering the driver core list of
PCI devices, which is what really mattered.)

So everything should still be fine.

thanks,

greg k-h

Re: pci: add to global list before sysfs?

[Benjamin Herrenschmidt]

On Tue, 2008-04-15 at 12:13 -0700, Greg KH wrote:
> > Haven't had a chance to look at your changes yet, but I have memories of
> > being pretty happy with having that 2 step mechanism in PCI where the
> > driver core would only see the devices after fixup.
> 
> No, that is left alone, I just dropped the duplicate device list that
> was linked within the PCI devices themselves.  The only user of it was
> the "reorder the PCI devices" code for some Dell machines, and that was
> just doing it to be safe (it was also reordering the driver core list of
> PCI devices, which is what really mattered.)
> 
> So everything should still be fine.

Ok, thanks.

Cheers,
Ben.