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

Message-ID: <20110509153728.GA29762@kroah.com>
Date:	Mon, 9 May 2011 08:37:28 -0700
From:	Greg KH <greg@...ah.com>
To:	Rafał Miłecki <zajec5@...il.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

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