linux-kernel - Re: [Pcihpd-discuss] [PATCH 2/5] Construct one fakephp slot per pci slot

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200711141549.50433.eike-hotplug@sf-tec.de>
Date:	Wed, 14 Nov 2007 15:49:42 +0100
From:	Rolf Eike Beer <eike-hotplug@...tec.de>
To:	Alex Chiang <achiang@...com>
Cc:	pcihpd-discuss@...ts.sourceforge.net, gregkh@...e.de,
	kristen.c.accardi@...el.com, lenb@...nel.org, matthew@....cx,
	rick.jones2@...com, linux-kernel@...r.kernel.org,
	linux-pci@...ey.karlin.mff.cuni.cz, linux-acpi@...r.kernel.org
Subject: Re: [Pcihpd-discuss] [PATCH 2/5] Construct one fakephp slot per pci slot

Am Mittwoch, 14. November 2007 schrieb Alex Chiang:
> Hi Eike,
>
> * Rolf Eike Beer <eike-hotplug@...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

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