linux-kernel - Re: [PATCH][WAS:bcmai, axi] bcma: add Broadcom specific AMBA bus driver

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] [day] [month] [year] [list]

Message-ID: <BANLkTi=08EDyiXPV1pPJzsjMF7JDXjkK-A@mail.gmail.com>
Date:	Mon, 9 May 2011 17:48:14 +0200
From:	Rafał Miłecki <zajec5@...il.com>
To:	Greg KH <greg@...ah.com>
Cc:	Arnd Bergmann <arnd@...db.de>,
	linux-arm-kernel@...ts.infradead.org,
	George Kashperko <george@...u.edu.ua>,
	Russell King <rmk@....linux.org.uk>,
	linux-wireless@...r.kernel.org,
	"John W. Linville" <linville@...driver.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	b43-dev@...ts.infradead.org,
	Michael Büsch <mb@...sch.de>,
	Arend van Spriel <arend@...adcom.com>,
	linuxdriverproject <devel@...uxdriverproject.org>,
	Andy Botting <andy@...ybotting.com>,
	Larry Finger <Larry.Finger@...inger.net>
Subject: Re: [PATCH][WAS:bcmai, axi] bcma: add Broadcom specific AMBA bus driver

2011/5/9 Greg KH <greg@...ah.com>:
> On Mon, May 09, 2011 at 04:33:43PM +0200, Rafał Miłecki wrote:
>> 2011/5/8 Arnd Bergmann <arnd@...db.de>:
>> > On Sunday 08 May 2011 16:59:55 Rafał Miłecki wrote:
>> >> 2011/5/8 Arnd Bergmann <arnd@...db.de>:
>> >> >> >> diff --git a/drivers/bcma/TODO b/drivers/bcma/TODO
>> >> >> >> new file mode 100644
>> >> >> >> index 0000000..45eadc9
>> >> >> >> --- /dev/null
>> >> >> >> +++ b/drivers/bcma/TODO
>> >> >> >> @@ -0,0 +1,3 @@
>> >> >> >> +- Interrupts
>> >> >> >> +- Defines for PCI core driver
>> >> >> >> +- Convert bcma_bus->cores into linked list
>> >> >> >
>> >> >> > The last item doesn't make sense to me. Since you are using the regular
>> >> >> > driver model, you can simply iterate over all child devices of any
>> >> >> > dev.
>> >> >>
>> >> >> It's about optimization. Right now bcma_bus->cores is static array, we
>> >> >> probably never will use all entries.
>> >> >
>> >> > Oh, I see. You should probably have neither of them. Instead allocate
>> >> > the devices dynamically as you find them and do a device_register,
>> >> > which will add the device into linked list.
>> >>
>> >> As I said, and wrote: TODO.
>> >
>> > Well, I think getting this part right is essential before the
>> > patch can get merged.
>> >
>> >> > Maybe you didn't understand what I said: This should be
>> >> >
>> >> > struct bcma_device {
>> >> >     struct bcma_bus *bus;
>> >> >     struct bcma_device_id id;
>> >> >     struct device dev;
>> >> >     u8 core_index;
>> >> >
>> >> >     u32 addr;
>> >> >     u32 wrap;
>> >> >
>> >> >     void *drvdata;
>> >> > };
>> >> >
>> >> > Here, bcma_device is the device, no need to follow pointers
>> >> > around. It's how all bus_types work, you should just do the same.
>> >>
>> >> We can not use static "struct device", see Greg's comments in:
>> >> [RFC][PATCH V3] axi: add AXI bus driver
>> >> (not to mention we would have unused "struct device" in ChipCommon's
>> >> and PCI's "struct bcma_device").
>> >
>> > Please reread what Greg explained, it's actually the same as what
>> > I said here: Don't make the device static (you already don't),
>> > don't put the device structure as a member in the bus structure
>> > (as discussed above). Make the device a member of bcma_device,
>> > so you get proper reference counting for it, in the way that
>> > Greg explained.
>>
>> Thanks for help & explaining! Unfortunately Greg didn't answer if my
>> changed implementation is fine. I'll fix this!
>
> Greg didn't know that you changed it, or that you wanted review comments
> on it.
>
> thanks,
>
> greg "please be specific when asking for review" k-h

Ups :) I asked for opinion in:
Date: Tue, 12 Apr 2011 20:52:55 +0200
Message-Id: <1302634375-2378-1-git-send-email-zajec5@...il.com>

> Greg: is this what you expected from dynamic allocation and documentation?


But it was hidden in commit comment and there were so many e-mails, it
was probably easy to do not notice it.

-- 
Rafał
--
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/