LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alex Chiang <achiang@hp.com>
To: Greg KH <gregkh@suse.de>
Cc: Gary Hade <garyhade@us.ibm.com>,
	kaneshige.kenji@jp.fujitsu.com, warthog19@eaglescrag.net,
	Matthew Wilcox <matthew@wil.cx>,
	kristen.c.accardi@intel.com, rick.jones2@hp.com,
	linux-kernel@vger.kernel.org, linux-pci@atrey.karlin.mff.cuni.cz,
	linux-acpi@vger.kernel.org
Subject: Re: [PATCH 3/4] Introduce pci_slot
Date: Mon, 3 Mar 2008 13:56:21 -0700	[thread overview]
Message-ID: <20080303205621.GD17814@ldl.fc.hp.com> (raw)
In-Reply-To: <20080301052433.GC19353@suse.de>

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.

  - 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).

> oh, and don't put "extern" in a .c file, 

Fixed in next version.

> 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?

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.

Your thoughts?

Thanks.

/ac


  reply	other threads:[~2008-03-03 20:56 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-29  0:23 [PATCH 0/4, v7] PCI, ACPI: Physical PCI slot objects Alex Chiang
2008-02-29  0:26 ` [PATCH 1/4] Remove path attribute from sgi_hotplug Alex Chiang
2008-03-03 18:48   ` Jesse Barnes
2008-03-03 18:54     ` Prarit Bhargava
2008-03-05  0:19       ` Alex Chiang
2008-02-29  0:27 ` [PATCH 2/4] Construct one fakephp slot per pci slot Alex Chiang
2008-02-29  0:28 ` [PATCH 3/4] Introduce pci_slot Alex Chiang
2008-03-01  5:24   ` Greg KH
2008-03-03 20:56     ` Alex Chiang [this message]
2008-03-04  5:58       ` Greg KH
2008-03-04 23:30   ` [PATCH 3/4, v8] " Alex Chiang
2008-02-29  0:29 ` [PATCH 4/4] ACPI PCI slot detection driver Alex Chiang
2008-03-01  5:25   ` Greg KH
2008-03-01 14:43     ` Matthew Wilcox
2008-03-04  5:49       ` Greg KH
2008-03-04 18:18         ` Jesse Barnes
2008-03-04 19:30           ` Greg KH
2008-03-04 20:02             ` Jesse Barnes
2008-03-04 20:12             ` Kristen Carlson Accardi
2008-03-04 23:09             ` Alex Chiang
2008-03-05  1:11               ` Kenji Kaneshige
2008-03-05 20:20                 ` Alex Chiang
2008-03-05 20:34                   ` Matthew Wilcox
2008-03-06  2:07                   ` Kenji Kaneshige
2008-03-11 13:10                   ` Kenji Kaneshige
2008-03-11 13:13                     ` [PATCH 3/(3+1)] " Kenji Kaneshige
2008-03-11 13:17                       ` Kenji Kaneshige
2008-03-11 13:19                     ` [PATCH 4/(3+1)] Add quirks for " Kenji Kaneshige
2008-03-11 13:28                     ` [PATCH 4/4] " Matthew Wilcox
2008-03-11 16:56                       ` Jesse Barnes
2008-03-12  5:51                         ` Kenji Kaneshige
2008-03-12  4:08                       ` Kenji Kaneshige
2008-03-11 18:04                     ` Kristen Carlson Accardi
2008-03-11 19:14                       ` Alex Chiang
2008-03-12 11:33                         ` Kenji Kaneshige
2008-03-13  3:24                           ` Alex Chiang
2008-03-14  2:16                             ` Gary Hade
2008-03-14  5:34                               ` Kenji Kaneshige
2008-03-18 20:49                                 ` Alex Chiang
2008-03-12 10:50                       ` Kenji Kaneshige
2008-03-11 23:34                     ` Kristen Carlson Accardi
2008-03-12 12:59                       ` Kenji Kaneshige
2008-03-04 22:58         ` Alex Chiang
2008-03-04 23:15           ` Greg KH
2008-03-04 23:46             ` Alex Chiang
2008-03-01  5:12 ` [PATCH 0/4, v7] PCI, ACPI: Physical PCI slot objects Greg KH
2008-03-03 23:35   ` Alex Chiang
2008-03-04  5:56     ` Greg KH
2008-03-25  4:13 [PATCH 0/4, v11] " Alex Chiang
2008-03-25  4:17 ` [PATCH 3/4] Introduce pci_slot Alex Chiang
2008-03-25  4:49   ` Kenji Kaneshige
2008-03-28 10:39   ` Andrew Morton
2008-03-28 15:41     ` Alex Chiang
2008-03-28 17:56       ` Andrew Morton
2008-03-25 17:09 [PATCH 0/4, v12] PCI, ACPI: Physical PCI slot objects Alex Chiang
2008-03-25 17:13 ` [PATCH 3/4] Introduce pci_slot Alex Chiang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080303205621.GD17814@ldl.fc.hp.com \
    --to=achiang@hp.com \
    --cc=garyhade@us.ibm.com \
    --cc=gregkh@suse.de \
    --cc=kaneshige.kenji@jp.fujitsu.com \
    --cc=kristen.c.accardi@intel.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@atrey.karlin.mff.cuni.cz \
    --cc=matthew@wil.cx \
    --cc=rick.jones2@hp.com \
    --cc=warthog19@eaglescrag.net \
    --subject='Re: [PATCH 3/4] Introduce pci_slot' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).