Re: [PATCH 3/4] Introduce pci_slot

[Greg KH <gregkh@suse.de>]

On Mon, Mar 03, 2008 at 01:56:21PM -0700, Alex Chiang wrote:
> Hi Greg,
> 
> * Greg KH <gregkh@suse.de>:
> > On Thu, Feb 28, 2008 at 05:28:55PM -0700, Alex Chiang wrote:
> > >   - Make pci_slot the primary sysfs entity. hotplug_slot becomes a
> > >     subsidiary structure.
> > >     o pci_create_slot() creates and registers a slot with the PCI core
> > >     o pci_slot_add_hotplug() gives it hotplug capability
> > > 
> > >   - Change the prototype of pci_hp_register() to take the bus and
> > >     slot number (on parent bus) as parameters.
> > > 
> > >   - Remove all the ->get_address methods since this functionality is
> > >     now handled by pci_slot directly.
> > 
> > This describes what you did, but not why you are doing this, making it a
> > pretty bad changelog comment.
> > 
> > Can you refresh my memory as to the "why" for all of this 
> 
> How about this:
> 
> Currently, /sys/bus/pci/slots/ only exposes hotplug attributes
> when a hotplug driver is loaded, but PCI slots have attributes
> such as address, speed, width, etc. that are not related to
> hotplug at all.
> 
> Introduce pci_slot as the primary data structure and kobject
> model. Hotplug attributes described in hotplug_slot become a
> secondary structure associated with the pci_slot.
> 
> This patch only creates the infrastructure that allows the
> separation of PCI slot attributes and hotplug attributes.
> In this patch, the PCI hotplug core remains the only user of this
> infrastructure, and thus, /sys/bus/pci/slots/ will still only
> become populated when a hotplug driver is loaded.
> 
> A later patch in this series will add a second user of this new
> infrastructure and demonstrate splitting the task of exposing
> pci_slot attributes from hotplug_slot attributes.

Ok, that's a bit better, please include it in the changelog information
in the patch next time :)

>   - Make pci_slot the primary sysfs entity. hotplug_slot becomes a
>     subsidiary structure.
>     o pci_create_slot() creates and registers a slot with the PCI core
>     o pci_slot_add_hotplug() gives it hotplug capability
> 
>   - Change the prototype of pci_hp_register() to take the bus and
>     slot number (on parent bus) as parameters.
> 
>   - Remove all the ->get_address methods since this functionality is
>     now handled by pci_slot directly.
> 
> > and how you are handling machines that do not export this
> > information at all?
> 
> With this patch, there is no change from existing behavior that
> users see; only infrastructure is changed. The existing hotplug
> drivers will continue to expose whatever they were exposing
> before (when they are loaded).

The issue is for non-hotpluggable slots, right?  That's a big behavior
change, especially for userspace tools not expecting that.

> > and call kobject_uevent in the same function that you added the
> > kobject in, unless there is a very good reason to do so,
> > otherwise you just missed all of those events...
> 
> I *think* I might actually have a "good" reason, but welcome your
> feedback.
> 
> In this patch, pci_create_slot() is responsible for
> kobject_init_and_add, and it adds the 'address' attribute in
> sysfs.
> 
> The caller of pci_create_slot() is pci_hp_register, and it is
> calling kobject_uevent after pci_create_slot, because it still
> has to expose the hotplug attributes in sysfs, which can only
> happen *after* the pci_slot is created.
> 
> I don't think I want to emit the uevent until those hotplug
> attributes are exposed, right?

That is correct, but the location seemed a bit "odd", next time around
I'll review it again to be sure.

> This kinda seems like a stupid design, but the next patch in the
> series adds another callsite for pci_create_slot. The next patch
> is detecting physical slots described by ACPI, but doesn't know
> (or care) about their hotplug capabilities.
> 
> I don't think it makes sense to be emitting uevents simply upon
> detecting a physical slot.
> 
> [over time, I hope to add more functionality to pci_slot, such as
> displaying speed, width, etc., but right now, we only get the
> address]
> 
> One alternative I can think of -- which would further complicate
> this model that I'm introducing -- would be to make hotplug_slot
> a kobject too, and then let pci_slot emit a uevent upon physical
> slot detection, and then let pci_hp_register emit another uevent
> when the hotplug_slot is created / added to sysfs. But I must
> admit, I don't really like that alternative.

No, that's a mess, don't do that :)

thanks,

greg k-h