lkml.org 
[lkml]   [2007]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [Pcihpd-discuss] [PATCH 2/5] Construct one fakephp slot per pci slot
    Date
    Am Mittwoch, 14. November 2007 schrieb Alex Chiang:
    > Hi Eike,
    >
    > * Rolf Eike Beer <eike-hotplug@sf-tec.de>:
    > > Alex Chiang wrote:
    > > > --- a/drivers/pci/hotplug/fakephp.c
    > > > +++ b/drivers/pci/hotplug/fakephp.c
    > > > @@ -93,6 +93,7 @@ static int add_slot(struct pci_dev *dev)
    > > > struct dummy_slot *dslot;
    > > > struct hotplug_slot *slot;
    > > > int retval = -ENOMEM;
    > > > + static int count = 1;
    > > >
    > > > slot = kzalloc(sizeof(struct hotplug_slot), GFP_KERNEL);
    > > > if (!slot)
    > > > @@ -106,7 +107,8 @@ static int add_slot(struct pci_dev *dev)
    > > > slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
    > > > slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
    > > >
    > > > - slot->name = &dev->dev.bus_id[0];
    > > > + slot->name = kmalloc(8, GFP_KERNEL);
    > > > + sprintf(slot->name, "fake%d", count++);
    > > > dbg("slot->name = %s\n", slot->name);
    > > >
    > > > dslot = kmalloc(sizeof(struct dummy_slot), GFP_KERNEL);
    > >
    > > This is ugly. Please do it the way we already do e.g. for
    > > acpiphp: add a char[8] to "struct dummy_slot" and just
    > > reference that here.
    >
    > I took at look at the code in acpiphp you're talking about, and
    > I'm not sure why you think the above is "ugly". We're talking
    > about a runtime vs compile time storage for the name, and doing a
    > kmalloc/sprintf is a pretty standard idiom.
    >
    > BTW, I did incorporate both Linas' and Willy's comments, by
    > changing the kmalloc size to an explicit 32, and using snprintf
    > instead.
    >
    > In any case, for your particular comment, I think I'm going to
    > leave it alone for now, and let Greg weigh in with the final
    > recommendation.

    Because we have another allocation of very small size for every slot here.

    struct dummy_slot has a size of 4 pointers, that's 16 byte for 32 bit
    architectures. Putting 8 byte of additional storage here would save a
    complete allocation on 32 bit platforms. For 64 bit platforms the memory
    usage is the same but we do less allocations (i.e. less points to fail) and
    less memory fragmentation.

    Btw: your code lacks a check if kmalloc() returns NULL.

    Eike
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2007-11-14 15:53    [W:4.708 / U:0.100 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site