Re: [PATCH 1/2] device: Stop requiring that struct device is embedded in struct pci_dev

[gregkh@xxxxxxxxxxxxxxxxxxx]

On Tue, Mar 07, 2017 at 04:54:58PM +0000, Bart Van Assche wrote:
> On Tue, 2017-03-07 at 05:52 +0100, Greg Kroah-Hartman wrote:
> > Somehow all other subsystems work just fine, don't instantly think that
> > the driver core needs to bend to the will of the IB code, because you
> > are somehow "special".  Hint, you aren't :)
> 
> Hi Greg,
> 
> In another e-mail Parav compared IB drivers with networking drivers.

Great, then notice that networking drivers don't need to do this type of
crud :)

> But I think that's a bad comparison: in the networking stack it's the
> network driver itself that sets up and triggers DMA while in the IB
> stack it's the upper layer protocol (ULP) driver that calls the
> functions defined in struct dma_ops. For some IB HW drivers (hfi1, qib
> and rdma_rxe) the ULP driver must
> use the DMA mapping operations from lib/dma-virt.c while for all other IB HW
> drivers the ULP driver must use the PCI DMA mapping functions. The ib_dma_*()
> functions select the right DMA mapping operations - either the PCI DMA
> mapping operations or those from lib/dma-virt.c. My question to you is how we
> should organize struct ib_device such that we can get rid of the ib_dma_*()
> helper functions. How to make sure that the to_pci_dev() translation works
> correctly for the device structure that is embedded in struct ib_device?
> Should a pointer to struct pci_dev be embedded in struct device (as done in
> patch 1/2 in this series)

I already said no to this, why do you think that it is still ok?

> or should the struct device in ib_device be changed
> into a struct pci_dev

Ick, no.

> and should the pci_dev information from /sys/devices/pci*/*/* be
> duplicated into the pci_dev information in struct ib_device
> (/sys/devices/pci*/*/*/infiniband/*)?

I don't think you really thought that one through :)

> For the latter approach, would
> there be a risk that the duplicated information becomes inconsistent?

No, it just wouldn't work :)

Why not just save off a pointer to your pci_dev in your ib_device
structure?  That way you know what the type is, and you have access to
everything you need.

But hey, I know nothing about IB and I really want to keep it that way.
You do what you want to, as long as you don't abuse the driver model,
like your patch 1/2 did.  Remember, not all the world is IB.

thanks,

greg k-h