Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()

[Liviu Dudau <liviu@dudau.co.uk>]

On Wed, Nov 19, 2014 at 10:24:52AM +0800, Yijing Wang wrote:
> >> We need, some platforms pass NULL pointer as host bridge parent.
> > 
> > Yijing,
> > 
> > May I suggest a different approach here? Rather than having to pass an opaque
> > pointer that gets converted by the host bridge driver back to the private
> > structure, what about promoting a new style of usage, that is similar to the
> > way device drivers work? Lets try to promote the embedding of the generic
> > pci_host_bridge structure in the host bridge specific structure! Then we can
> > access the private data doing container_of().
> > 
> > Something like this:
> > 
> > struct pci_controller {
> > 	struct pci_host_bridge bridge;
> > 	/* private host bridge data here */
> > 	.....
> > };
> > 
> > #define PCI_CONTROLLER(bus)	({
> > 	struct pci_host_bridge *hb = to_pci_host_bridge(bus->bridge); \
> > 	container_of(hb, struct pci_controller, bridge); })
> > 
> > 
> > Then we can retrieve the host bridge structure from everywhere we have a device.
> 
> Hi Liviu, it looks good to me, because this change will involve lots platforms,
> I would think more about it. Thanks for your suggestion very much! :)

Given that I also look at this area maybe we should join forces and divide the problem?

Best regards,
Liviu

> 
> 
> Thanks!
> Yijing.
> 
> > 
> > Best regards,
> > Liviu
> > 
> >>
> >>>
> >>>> +	host = kzalloc(sizeof(*host), GFP_KERNEL);
> >>>> +	if (!host)
> >>>> +		return NULL;
> >>>
> >>> devm_kzalloc maybe?
> >>
> >> I don't know much detail about devm_kzalloc(), but we have no pci host driver
> >> here, and I found no devm_kzalloc() uses in core PCI code before.
> >>
> >>>
> >>>> +	if (!resources) {
> >>>> +		/* Use default IO/MEM/BUS resources*/
> >>>> +		pci_add_resource(&host->windows, &ioport_resource);
> >>>> +		pci_add_resource(&host->windows, &iomem_resource);
> >>>> +		pci_add_resource(&host->windows, &busn_resource);
> >>>> +	} else {
> >>>> +		list_for_each_entry_safe(window, n, resources, list)
> >>>> +			list_move_tail(&window->list, &host->windows);
> >>>> +	}
> >>>
> >>> I think we should assume that the correct resources are passed. You
> >>> could add a wrapper around this function to convert old platforms
> >>> though.
> >>
> >> OK, I will move these code out of pci_create_host_bridge, and add a wrapper
> >> to setup the default resources.
> >>
> >>>
> >>>> +EXPORT_SYMBOL(pci_create_host_bridge);
> >>>
> >>> EXPORT_SYMBOL_GPL() maybe?
> >>
> >> OK, will update it.
> >>
> >>>
> >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >>>> index 8b11b38..daa7f40 100644
> >>>> --- a/include/linux/pci.h
> >>>> +++ b/include/linux/pci.h
> >>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window {
> >>>>  struct pci_host_bridge {
> >>>>  	struct device dev;
> >>>>  	struct pci_bus *bus;		/* root bus */
> >>>> +	struct list_head list;
> >>>>  	struct list_head windows;	/* pci_host_bridge_windows */
> >>>> +	int busnum;
> >>>
> >>> The busnum should already be implied through the bus resource.
> >>
> >> Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks!
> >>
> >> Thanks!
> >> Yijing.
> >>
> >>>
> >>> 	Arnd
> >>>
> >>> .
> >>>
> >>
> >>
> >> -- 
> >> Thanks!
> >> Yijing
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> > 
> 
> 
> -- 
> Thanks!
> Yijing
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...