| 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: <20080819183908.GD18842@ldl.fc.hp.com>
Date: Tue, 19 Aug 2008 12:39:08 -0600
From: Alex Chiang <achiang@...com>
To: Rolf Eike Beer <eike-kernel@...tec.de>
Cc: 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
* 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.
Also, it helps later on, when trying to stay below the 80 column
limit. :)
> > 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.
> > @@ -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>
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.
/ac
>
> > retval = pci_hp_register(slot->hotplug_slot,
> > acpiphp_slot->bridge->pci_bus,
> > acpiphp_slot->device,
> > - slot->name);
> > + name);
> > if (retval == -EBUSY)
> > goto error_hpslot;
> > if (retval) {
> > @@ -349,7 +351,7 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot
> > *acpiphp_slot) goto error_hpslot;
> > }
> >
> > - info("Slot [%s] registered\n", slot->hotplug_slot->name);
> > + info("Slot [%s] registered\n", slot_name(slot));
> >
> > return 0;
> > error_hpslot:
> > @@ -366,7 +368,7 @@ void acpiphp_unregister_hotplug_slot(struct
> > acpiphp_slot *acpiphp_slot) struct slot *slot = acpiphp_slot->slot;
> > int retval = 0;
> >
> > - info ("Slot [%s] unregistered\n", slot->hotplug_slot->name);
> > + info ("Slot [%s] unregistered\n", slot_name(slot));
> >
> > retval = pci_hp_deregister(slot->hotplug_slot);
> > if (retval)
>
> Greetings,
>
> Eike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists