Alex Chiang wrote:
> * Rolf Eike Beer <[email protected]>:
> > 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: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Signed-off-by: Alex Chiang <[email protected]>
> > > ---
> > >
> > > 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

Attachments:

(No filename) (9.10 kB)
signature.asc (197.00 B)
This is a digitally signed message part. Download all attachments

2008-08-19 21:27:32

by Alex Chiang

[permalink] [raw]

Subject: Re: [PATCH 04/13] PCI: acpiphp: remove 'name' parameter

* Rolf Eike Beer <[email protected]>:
> Alex Chiang wrote:
> > * Rolf Eike Beer <[email protected]>:
> > > Alex Chiang wrote:
> > > >
> > > > +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.

For example, in shpchp, I use slot_name() in both shpchp_core.c
and in shpchp_ctrl.c. This is the case for a few other drivers
too.

Based on that reasoning, I felt a little consistency across
drivers was warranted. Besides, the generated code should be the
same.

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

Ok, you're right. For some reason, I was thinking that there were
versions of snprintf() that did not NULL-terminate in the event
of truncation, but I checked the kernel implementation, and it
does NULL terminate in either case.

Besides, it looks like I should be using scnprintf() instead. So
I'll rework the patches.

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

The #define exists in the original pcihp_skeleton.c example.

This is perhaps the 2nd or 3rd time I've swept through the
PCI hotplug drivers for cleanups, and for me, I really prefer
some sort of consistency, otherwise I go insane trying to keep
track of the differences.

But I will defer to Jesse's opinion.

Thanks,

/ac

2008-08-19 21:40:48

by Jesse Barnes

[permalink] [raw]

Subject: Re: [PATCH 04/13] PCI: acpiphp: remove 'name' parameter

On Tuesday, August 19, 2008 11:39 am Alex Chiang wrote:
> * Rolf Eike Beer <[email protected]>:
> > Alex Chiang wrote:
> > > +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.
>
> Also, it helps later on, when trying to stay below the 80 column
> limit. :)

If they're all identical, maybe we should pull this up into a common function
(with a short name :). That can be a subsequent cleanup though.

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

Yeah the main reason for separating whitespace changes from real ones is to
avoid making the patch hard to read in case a problem crops up. Killing an
extra newline doesn't make things harder to read, imo, so it's fine with me
if you keep it (though if you *do* end up doing a cleanup patch later you
could save it for that).

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

kerneldoc is ok with either */ or **/ as a closing comment, so you can drop
these bits.

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

Yeah, but you'll overwrite it with snprintf anyway, so the memset is
redundant.

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

Going from the define to sizeof() everywhere could be another cleanup, but I
don't have strong feelings about that.

Jesse

2008-08-19 23:50:38

by Alex Chiang

[permalink] [raw]

Subject: Re: [PATCH 04/13] PCI: acpiphp: remove 'name' parameter

* Jesse Barnes <[email protected]>:
> On Tuesday, August 19, 2008 11:39 am Alex Chiang wrote:
> > * Rolf Eike Beer <[email protected]>:
> > > Alex Chiang wrote:
> > > > +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.
> >
> > Also, it helps later on, when trying to stay below the 80 column
> > limit. :)
>
> If they're all identical, maybe we should pull this up into a
> common function (with a short name :). That can be a
> subsequent cleanup though.

Ok, I'll save this for next time. Or someone else can have the
git glory. ;)

> > > > @@ -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.
>
> Yeah the main reason for separating whitespace changes from
> real ones is to avoid making the patch hard to read in case a
> problem crops up. Killing an extra newline doesn't make things
> harder to read, imo, so it's fine with me if you keep it
> (though if you *do* end up doing a cleanup patch later you
> could save it for that).

I'll keep the 'kill the newline' here but drop the kerneldoc
stuff. As you point out, kerneldoc will take either */ or **/.

> > > > 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>
>
> Yeah, but you'll overwrite it with snprintf anyway, so the memset is
> redundant.

Ok. I went through and removed the superfluous memsets, and
changed instances of snprintf() to scnprintf().

> > On the other hand, keeping the #define is important, because
> > again, that's the established convention of the PCI hotplug
> > drivers.
> >
> > Thanks for the review.
>
> Going from the define to sizeof() everywhere could be another
> cleanup, but I don't have strong feelings about that.

Ok, saving these for next time too.

I'll hold off from sending v2 until I hear comments from
Kenji-san, so if he has any suggestions, I can incorporate them
all in v2.

Thanks.

/ac

2008-08-20 02:27:44

by Kenji Kaneshige

[permalink] [raw]

Subject: Re: [PATCH 04/13] PCI: acpiphp: remove 'name' parameter

Alex Chiang wrote:
> * Jesse Barnes <[email protected]>:
>> On Tuesday, August 19, 2008 11:39 am Alex Chiang wrote:
>>> * Rolf Eike Beer <[email protected]>:
>>>> Alex Chiang wrote:
>>>>> +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.
>>>
>>> Also, it helps later on, when trying to stay below the 80 column
>>> limit. :)
>> If they're all identical, maybe we should pull this up into a
>> common function (with a short name :). That can be a
>> subsequent cleanup though.
>
> Ok, I'll save this for next time. Or someone else can have the
> git glory. ;)
>
>>>>> @@ -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.
>> Yeah the main reason for separating whitespace changes from
>> real ones is to avoid making the patch hard to read in case a
>> problem crops up. Killing an extra newline doesn't make things
>> harder to read, imo, so it's fine with me if you keep it
>> (though if you *do* end up doing a cleanup patch later you
>> could save it for that).
>
> I'll keep the 'kill the newline' here but drop the kerneldoc
> stuff. As you point out, kerneldoc will take either */ or **/.
>
>>>>> 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>
>> Yeah, but you'll overwrite it with snprintf anyway, so the memset is
>> redundant.
>
> Ok. I went through and removed the superfluous memsets, and
> changed instances of snprintf() to scnprintf().
>
>>> On the other hand, keeping the #define is important, because
>>> again, that's the established convention of the PCI hotplug
>>> drivers.
>>>
>>> Thanks for the review.
>> Going from the define to sizeof() everywhere could be another
>> cleanup, but I don't have strong feelings about that.
>
> Ok, saving these for next time too.
>
> I'll hold off from sending v2 until I hear comments from
> Kenji-san, so if he has any suggestions, I can incorporate them
> all in v2.
>

Hi Alex-san, and all,

I'm sorry for my delay. I had been on vacation on these days.
I'll review your patches as soon as possible.

Thanks,
Kenji Kaneshige

2008-08-21 10:25:53

by Kenji Kaneshige

[permalink] [raw]

Subject: Re: [PATCH 02/13] PCI: prevent duplicate slot names

Hi Alex-san,

Thank you for updated patches and I'm sorry for delayed comment.
I reviewed your patch and I found two problems. Please see below.

Alex Chiang wrote:
> Prevent callers of pci_create_slot() from registering slots with
> duplicate names. This condition occurs most often when PCI hotplug
> drivers are loaded on platforms with broken firmware that assigns
> identical names to multiple slots.
>
> We now rename these duplicate slots on behalf of the user.
>
> If firmware assigns the name N to multiple slots, then:
>
> The first registered slot is assigned N
> The second registered slot is assigned N-1
> The third registered slot is assigned N-2
> The Mth registered slot becomes N-M
>
> A side effect of this patch is that the error condition for when
> multiple drivers attempt to claim the same slot becomes much more
> prominent.
>
> In other words, the previous error condition returned for
> duplicate slot names (-EEXIST) masked the case when multiple
> drivers attempted to claim the same slot. Now, the -EBUSY return
> makes the true error more obvious.
>
> Finally, since we now prevent duplicate slot names, we remove
> the logic introduced by the following commits:
>
> pci hotplug core: add check of duplicate slot name
> a86161b3134465f072d965ca7508ec9c1e2e52c7
>
> pciehp: fix slot name
> 3800345f723fd130d50434d4717b99d4a9f383c8
>
> pciehp: add message about pciehp_slot_with_bus option
> 9e4f2e8d4ddb04ad16a3828cd9a369a5a5287009
>
> shpchp: fix slot name
> ef0ff95f136f0f2d035667af5d18b824609de320
>
> shpchp: add message about shpchp_slot_with_bus option
> b3bd307c628af2f0a581c42d5d7e4bcdbbf64b6a
>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Alex Chiang <[email protected]>
> ---
>
> drivers/pci/hotplug/pci_hotplug_core.c | 4 -
> drivers/pci/hotplug/pciehp.h | 1
> drivers/pci/hotplug/pciehp_core.c | 7 --
> drivers/pci/hotplug/pciehp_hpc.c | 6 --
> drivers/pci/hotplug/shpchp_core.c | 14 -----
> drivers/pci/slot.c | 93 +++++++++++++++++++++++++-------
> 6 files changed, 76 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
> index 96f274e..da5908f 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -570,10 +570,6 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
> return -EINVAL;
> }
>
> - /* Check if we have already registered a slot with the same name. */
> - if (get_slot_from_name(name))
> - return -EEXIST;
> -
> /*
> * No problems if we call this interface from both ACPI_PCI_SLOT
> * driver and call it here again. If we've already created the
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index e3a1e7e..9e6cec6 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -43,7 +43,6 @@ extern int pciehp_poll_mode;
> extern int pciehp_poll_time;
> extern int pciehp_debug;
> extern int pciehp_force;
> -extern int pciehp_slot_with_bus;
> extern struct workqueue_struct *pciehp_wq;
>
> #define dbg(format, arg...) \
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 5952315..bed77af 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -41,7 +41,6 @@ int pciehp_debug;
> int pciehp_poll_mode;
> int pciehp_poll_time;
> int pciehp_force;
> -int pciehp_slot_with_bus;
> struct workqueue_struct *pciehp_wq;
>
> #define DRIVER_VERSION "0.4"
> @@ -56,12 +55,10 @@ module_param(pciehp_debug, bool, 0644);
> module_param(pciehp_poll_mode, bool, 0644);
> module_param(pciehp_poll_time, int, 0644);
> module_param(pciehp_force, bool, 0644);
> -module_param(pciehp_slot_with_bus, bool, 0644);
> MODULE_PARM_DESC(pciehp_debug, "Debugging mode enabled or not");
> MODULE_PARM_DESC(pciehp_poll_mode, "Using polling mechanism for hot-plug events or not");
> MODULE_PARM_DESC(pciehp_poll_time, "Polling mechanism frequency, in seconds");
> MODULE_PARM_DESC(pciehp_force, "Force pciehp, even if _OSC and OSHP are missing");
> -MODULE_PARM_DESC(pciehp_slot_with_bus, "Use bus number in the slot name");
>
> #define PCIE_MODULE_NAME "pciehp"
>
> @@ -226,10 +223,6 @@ static int init_slots(struct controller *ctrl)
> slot->name);
> if (retval) {
> err("pci_hp_register failed with error %d\n", retval);
> - if (retval == -EEXIST)
> - err("Failed to register slot because of name "
> - "collision. Try \'pciehp_slot_with_bus\' "
> - "module option.\n");
> goto error_info;
> }
> /* create additional sysfs entries */
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index ad27e9e..43ff979 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -1032,11 +1032,7 @@ static void pcie_shutdown_notification(struct controller *ctrl)
>
> static void make_slot_name(struct slot *slot)
> {
> - if (pciehp_slot_with_bus)
> - snprintf(slot->name, SLOT_NAME_SIZE, "%04d_%04d",
> - slot->bus, slot->number);
> - else
> - snprintf(slot->name, SLOT_NAME_SIZE, "%d", slot->number);
> + snprintf(slot->name, SLOT_NAME_SIZE, "%d", slot->number);
> }
>
> static int pcie_init_slot(struct controller *ctrl)
> diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
> index 7bc06c0..136d9ea 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -39,7 +39,6 @@
> int shpchp_debug;
> int shpchp_poll_mode;
> int shpchp_poll_time;
> -static int shpchp_slot_with_bus;
> struct workqueue_struct *shpchp_wq;
>
> #define DRIVER_VERSION "0.4"
> @@ -53,11 +52,9 @@ MODULE_LICENSE("GPL");
> module_param(shpchp_debug, bool, 0644);
> module_param(shpchp_poll_mode, bool, 0644);
> module_param(shpchp_poll_time, int, 0644);
> -module_param(shpchp_slot_with_bus, bool, 0644);
> MODULE_PARM_DESC(shpchp_debug, "Debugging mode enabled or not");
> MODULE_PARM_DESC(shpchp_poll_mode, "Using polling mechanism for hot-plug events or not");
> MODULE_PARM_DESC(shpchp_poll_time, "Polling mechanism frequency, in seconds");
> -MODULE_PARM_DESC(shpchp_slot_with_bus, "Use bus number in the slot name");
>
> #define SHPC_MODULE_NAME "shpchp"
>
> @@ -101,12 +98,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
>
> static void make_slot_name(struct slot *slot)
> {
> - if (shpchp_slot_with_bus)
> - snprintf(slot->hotplug_slot->name, SLOT_NAME_SIZE, "%04d_%04d",
> - slot->bus, slot->number);
> - else
> - snprintf(slot->hotplug_slot->name, SLOT_NAME_SIZE, "%d",
> - slot->number);
> + snprintf(slot->hotplug_slot->name, SLOT_NAME_SIZE, "%d", slot->number);
> }
>
> static int init_slots(struct controller *ctrl)
> @@ -162,10 +154,6 @@ static int init_slots(struct controller *ctrl)
> hotplug_slot->name);
> if (retval) {
> err("pci_hp_register failed with error %d\n", retval);
> - if (retval == -EEXIST)
> - err("Failed to register slot because of name "
> - "collision. Try \'shpchp_slot_with_bus\' "
> - "module option.\n");
> goto error_info;
> }
>
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 7e5b85c..42f0e12 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -73,6 +73,50 @@ static struct kobj_type pci_slot_ktype = {
> .default_attrs = pci_slot_default_attrs,
> };
>
> +static char *make_slot_name(const char *name)
> +{
> + char *new_name;
> + int len, width, dup = 1;
> + struct kobject *dup_slot;
> +
> + new_name = kstrdup(name, GFP_KERNEL);
> + if (!new_name)
> + goto out;
> +
> + /*
> + * Start off allocating enough room for "name-X"
> + */
> + len = strlen(name) + 2;
> + width = 1;
> +
> +try_again:
> + dup_slot = kset_find_obj(pci_slots_kset, new_name);
> + if (!dup_slot)
> + goto out;
> +

The kset_find_obj() increments reference counter of specified kobject. So
we must call kobject_put() for 'dup_slot', otherwise it leaks reference
counter of 'dup_slot' kobject and the corresponding slot directory will
never be removed. Here is a sample to fix this problem.

dup_slot = kset_find_ojb(pci_slots_kset, new_name);
if (!dup_slot)
goto out;
kobject_put(dup_slot);
^^^^^^^^^^^^^^^^^^^^^^ Added this line

I found another problem in pci_hp_register() that would be more complex
to fix than above-mentioned problem. Here is a pci_hp_register() with
all of your patch applied.

int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
const char *name)
{
(snip...)

/*
* No problems if we call this interface from both ACPI_PCI_SLOT
* driver and call it here again. If we've already created the
* pci_slot, the interface will simply bump the refcount.
*/
pci_slot = pci_create_slot(bus, slot_nr, name);
if (IS_ERR(pci_slot))
return PTR_ERR(pci_slot);

if (pci_slot->hotplug) {
dbg("%s: already claimed\n", __func__);
pci_destroy_slot(pci_slot);
return -EBUSY;
}

slot->pci_slot = pci_slot;
pci_slot->hotplug = slot;

/*
* Allow pcihp drivers to override the ACPI_PCI_SLOT name.
*/
if (strcmp(kobject_name(&pci_slot->kobj), name)) {
result = kobject_rename(&pci_slot->kobj, name);
if (result) {
pci_destroy_slot(pci_slot);
return result;
}
}

(snip...)
}

If name duplication was detected in pci_create_slot(), it renames the
slot name like 'N-1' and return successfully. Even though slot's kobject
name was registered as 'N-1', 'name' array still have 'N' at this point.
So the following 'if' statement becomes true unexpectedly.

/*
* Allow pcihp drivers to override the ACPI_PCI_SLOT name.
*/
if (strcmp(kobject_name(&pci_slot->kobj), name)) {

Then pci_hp_register() attempt to rename the slot name with 'N' again
by calling kobject_rename(), but it fails because there already exists
kobject with name 'N'. As a result, pci_hp_register() will fail.

Thanks,
Kenji Kaneshige