| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <200808192301.56532.eike-kernel@sf-tec.de>
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