Date:	Tue, 19 Aug 2008 23:01:55 +0200
From:	Rolf Eike Beer <eike-kernel@...tec.de>
To:	Alex Chiang <achiang@...com>, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, jbarnes@...tuousgeek.org,
	kristen.c.accardi@...el.com, kaneshige.kenji@...fujitsu.com
Subject: Re: [PATCH 04/13] PCI: acpiphp: remove 'name' parameter

Alex Chiang wrote:
> * Rolf Eike Beer <eike-kernel@...tec.de>:
> > Alex Chiang wrote:
> > > We do not need to manage our own name parameter, especially since
> > > the PCI core can change it on our behalf, in the case of duplicate
> > > slot names.
> > >
> > > Remove 'name' from acpiphp's version of struct slot.
> > >
> > > Cc: jbarnes@...tuousgeek.org
> > > Cc: kristen.c.accardi@...el.com
> > > Cc: kaneshige.kenji@...fujitsu.com
> > > Signed-off-by: Alex Chiang <achiang@...com>
> > > ---
> > >
> > >  drivers/pci/hotplug/acpiphp.h      |    9 +++++----
> > >  drivers/pci/hotplug/acpiphp_core.c |   36
> > > +++++++++++++++++++----------------- 2 files changed, 24 insertions(+),
> > > 21 deletions(-)
> > >
> > > diff --git a/drivers/pci/hotplug/acpiphp.h
> > > b/drivers/pci/hotplug/acpiphp.h index 5a58b07..f9e244d 100644
> > > --- a/drivers/pci/hotplug/acpiphp.h
> > > +++ b/drivers/pci/hotplug/acpiphp.h
> > > @@ -50,9 +50,6 @@
> > >  #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME ,
> > > ## arg) #define warn(format, arg...) printk(KERN_WARNING "%s: " format,
> > > MY_NAME , ## arg)
> > >
> > > -/* name size which is used for entries in pcihpfs */
> > > -#define SLOT_NAME_SIZE	20		/* {_SUN} */
> > > -
> > >  struct acpiphp_bridge;
> > >  struct acpiphp_slot;
> > >
> > > @@ -63,9 +60,13 @@ struct slot {
> > >  	struct hotplug_slot	*hotplug_slot;
> > >  	struct acpiphp_slot	*acpi_slot;
> > >  	struct hotplug_slot_info info;
> > > -	char name[SLOT_NAME_SIZE];
> > >  };
> > >
> > > +static inline const char *slot_name(struct slot *slot)
> > > +{
> > > +	return hotplug_slot_name(slot->hotplug_slot);
> > > +}
> > > +
> > >  /*
> > >   * struct acpiphp_bridge - PCI bridge information
> > >   *
> >
> > I don't see a point in this function. Why not call hotplug_slot_name()
> > directly?
>
> You're correct that we don't exactly need it in acpiphp. However,
> it is a useful helper function for some of the other drivers, and
> I thought it would be better to keep consistency if possible.

I looked into all other patches and the function is the same in every one.

> Also, it helps later on, when trying to stay below the 80 column
> limit. :)

Rip it.

> > > diff --git a/drivers/pci/hotplug/acpiphp_core.c
> > > b/drivers/pci/hotplug/acpiphp_core.c index e984176..687cac3 100644
> > > --- a/drivers/pci/hotplug/acpiphp_core.c
> > > +++ b/drivers/pci/hotplug/acpiphp_core.c
> > > @@ -44,6 +44,9 @@
> > >
> > >  #define MY_NAME	"acpiphp"
> > >
> > > +/* name size which is used for entries in pcihpfs */
> > > +#define SLOT_NAME_SIZE  20              /* {_SUN} */
> > > +
> > >  static int debug;
> > >  int acpiphp_debug;
> > >
> > > @@ -84,7 +87,6 @@ static struct hotplug_slot_ops acpi_hotplug_slot_ops
> > > = { .get_adapter_status	= get_adapter_status,
> > >  };
> > >
> > > -
> > >  /**
> > >   * acpiphp_register_attention - set attention LED callback
> > >   * @info: must be completely filled with LED callbacks
> >
> > Fuzz.
>
> Yes, it's fuzz, but my practice has been to clean up* source files
> during the course of making actual, functional changes. Better
> than sending a mostly-useless whitespace patchbomb, IMO.
>
> * Note that "clean up" here means "reasonable cleanup" that
>   doesn't detract from reading the rest of the patch.
>
> > > @@ -92,7 +94,7 @@ static struct hotplug_slot_ops acpi_hotplug_slot_ops
> > > = { * Description: This is used to register a hardware specific ACPI *
> > > driver that manipulates the attention LED.  All the fields in * info
> > > must be set.
> > > - */
> > > + **/
> > >  int acpiphp_register_attention(struct acpiphp_attention_info *info)
> > >  {
> > >  	int retval = -EINVAL;
> >
> > Fuzz. Make the patch look bigger than it actually is. But that's just a
> > note, Jesse will have to judge if this is acceptable.
>
> See above. Fixing incorrect kerneldoc doesn't seem so bad, IMO.

As I said: Jesse's decision. The smaller the patch is the easier get's the 
review.

> > > @@ -113,7 +115,7 @@ int acpiphp_register_attention(struct
> > > acpiphp_attention_info *info) * Description: This is used to
> > > un-register a hardware specific acpi * driver that manipulates the
> > > attention LED.  The pointer to the * info struct must be the same as
> > > the one used to set it. - */
> > > + **/
> > >  int acpiphp_unregister_attention(struct acpiphp_attention_info *info)
> > >  {
> > >  	int retval = -EINVAL;
> > > @@ -136,7 +138,7 @@ static int enable_slot(struct hotplug_slot
> > > *hotplug_slot) {
> > >  	struct slot *slot = hotplug_slot->private;
> > >
> > > -	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
> > > +	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
> > >
> > >  	/* enable the specified slot */
> > >  	return acpiphp_enable_slot(slot->acpi_slot);
> > > @@ -154,7 +156,7 @@ static int disable_slot(struct hotplug_slot
> > > *hotplug_slot) struct slot *slot = hotplug_slot->private;
> > >  	int retval;
> > >
> > > -	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
> > > +	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
> > >
> > >  	/* disable the specified slot */
> > >  	retval = acpiphp_disable_slot(slot->acpi_slot);
> > > @@ -177,7 +179,7 @@ static int disable_slot(struct hotplug_slot
> > > *hotplug_slot) {
> > >  	int retval = -ENODEV;
> > >
> > > - 	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
> > > +	dbg("%s - physical_slot = %s\n", __func__,
> > > hotplug_slot_name(hotplug_slot));
> > >
> > >  	if (attention_info && try_module_get(attention_info->owner)) {
> > >  		retval = attention_info->set_attn(hotplug_slot, status);
> > > @@ -200,7 +202,7 @@ static int get_power_status(struct hotplug_slot
> > > *hotplug_slot, u8 *value) {
> > >  	struct slot *slot = hotplug_slot->private;
> > >
> > > -	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
> > > +	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
> > >
> > >  	*value = acpiphp_get_power_status(slot->acpi_slot);
> > >
> > > @@ -222,7 +224,7 @@ static int get_attention_status(struct hotplug_slot
> > > *hotplug_slot, u8 *value) {
> > >  	int retval = -EINVAL;
> > >
> > > -	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
> > > +	dbg("%s - physical_slot = %s\n", __func__,
> > > hotplug_slot_name(hotplug_slot));
> > >
> > >  	if (attention_info && try_module_get(attention_info->owner)) {
> > >  		retval = attention_info->get_attn(hotplug_slot, value);
> > > @@ -245,7 +247,7 @@ static int get_latch_status(struct hotplug_slot
> > > *hotplug_slot, u8 *value) {
> > >  	struct slot *slot = hotplug_slot->private;
> > >
> > > -	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
> > > +	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
> > >
> > >  	*value = acpiphp_get_latch_status(slot->acpi_slot);
> > >
> > > @@ -265,7 +267,7 @@ static int get_adapter_status(struct hotplug_slot
> > > *hotplug_slot, u8 *value) {
> > >  	struct slot *slot = hotplug_slot->private;
> > >
> > > -	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
> > > +	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
> > >
> > >  	*value = acpiphp_get_adapter_status(slot->acpi_slot);
> > >
> > > @@ -299,7 +301,7 @@ static void release_slot(struct hotplug_slot
> > > *hotplug_slot) {
> > >  	struct slot *slot = hotplug_slot->private;
> > >
> > > -	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
> > > +	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
> > >
> > >  	kfree(slot->hotplug_slot);
> > >  	kfree(slot);
> > > @@ -310,6 +312,7 @@ int acpiphp_register_hotplug_slot(struct
> > > acpiphp_slot *acpiphp_slot) {
> > >  	struct slot *slot;
> > >  	int retval = -ENOMEM;
> > > +	char name[SLOT_NAME_SIZE];
> > >
> > >  	slot = kzalloc(sizeof(*slot), GFP_KERNEL);
> > >  	if (!slot)
> > > @@ -321,8 +324,6 @@ int acpiphp_register_hotplug_slot(struct
> > > acpiphp_slot *acpiphp_slot)
> > >
> > >  	slot->hotplug_slot->info = &slot->info;
> > >
> > > -	slot->hotplug_slot->name = slot->name;
> > > -
> > >  	slot->hotplug_slot->private = slot;
> > >  	slot->hotplug_slot->release = &release_slot;
> > >  	slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;
> > > @@ -336,12 +337,13 @@ int acpiphp_register_hotplug_slot(struct
> > > acpiphp_slot *acpiphp_slot) slot->hotplug_slot->info->cur_bus_speed =
> > > PCI_SPEED_UNKNOWN;
> > >
> > >  	acpiphp_slot->slot = slot;
> > > -	snprintf(slot->name, sizeof(slot->name), "%u", slot->acpi_slot->sun);
> > > +	memset(name, 0, SLOT_NAME_SIZE);
> > > +	snprintf(name, SLOT_NAME_SIZE, "%u", slot->acpi_slot->sun);
> >
> > The memset() is not needed at all. And the sizeof is IMHO a good idea
> > anyway as it allows to get rid of the define.
>
> Hm, don't need a memset? I won't have garbage on the stack?
> </n00b>

Yes, you have garbage on the stack. But snprintf() does not care what is in 
the buffer before it starts and the result is 0-terminated afterwards.

> On the other hand, keeping the #define is important, because
> again, that's the established convention of the PCI hotplug
> drivers.

I would not bet on this. It has been there and copied around from one driver 
to the other. If we can get rid of those I guess noone would be upset. And 
you introduced at least two of them ;) Jesse? 

Eike

Download attachment "signature.asc " of type "application/pgp-signature" (198 bytes)

Powered by blists - more mailing lists