Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1593910 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] device: Stop requiring that struct device is  embedded in struct pci_dev Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-03-07 06:00 +0100
    RE: [PATCH 1/2] device: Stop requiring that struct device is embedded  in struct pci_dev Parav Pandit <parav@mellanox.com> - 2017-03-07 06:20 +0100
      RE: [PATCH 1/2] device: Stop requiring that struct device is embedded  in struct pci_dev Parav Pandit <parav@mellanox.com> - 2017-03-07 09:00 +0100
    RE: [PATCH 1/2] device: Stop requiring that struct device is embedded  in struct pci_dev Parav Pandit <parav@mellanox.com> - 2017-03-07 19:30 +0100
    Re: [PATCH 1/2] device: Stop requiring that struct device is  embedded in struct pci_dev "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org> - 2017-03-07 20:30 +0100
    Re: [PATCH 1/2] device: Stop requiring that struct device is  embedded in struct pci_dev Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-03-08 04:50 +0100

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

On Mon, Mar 06, 2017 at 04:35:48PM -0800, Bart Van Assche wrote:
> The dma mapping operations of several architectures and also of
> several I/O MMU implementations need to translate a struct
> device pointer into a struct pci_dev pointer. This translation
> is performed by to_pci_dev(). That macro assumes that struct
> device is embedded in struct pci_dev. However, that is not the
> case for the device structure in struct ib_device.

Then don't blindly cast it backwards!  Fix that up, an ib device should
have access to the dma structures that the PCI device it depends on has.
If not, you need to set that up properly in the IB core, don't mess with
the driver core for this at all.

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 :)

greg k-h

[toc] | [next] | [standalone]

#1593911 — RE: [PATCH 1/2] device: Stop requiring that struct device is embedded in struct pci_dev

Hi Greg,

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Monday, March 6, 2017 10:53 PM
> To: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Doug Ledford <dledford@redhat.com>; Sebastian Ott
> <sebott@linux.vnet.ibm.com>; Parav Pandit <parav@mellanox.com>; linux-
> rdma@vger.kernel.org; linux-kernel@vger.kernel.org; Bjorn Helgaas
> <bhelgaas@google.com>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; David Woodhouse <dwmw2@infradead.org>;
> H . Peter Anvin <hpa@zytor.com>; Ingo Molnar <mingo@redhat.com>;
> Russell King <linux@armlinux.org.uk>
> Subject: Re: [PATCH 1/2] device: Stop requiring that struct device is
> embedded in struct pci_dev
> 
> On Mon, Mar 06, 2017 at 04:35:48PM -0800, Bart Van Assche wrote:
> > The dma mapping operations of several architectures and also of
> > several I/O MMU implementations need to translate a struct device
> > pointer into a struct pci_dev pointer. This translation is performed
> > by to_pci_dev(). That macro assumes that struct device is embedded in
> > struct pci_dev. However, that is not the case for the device structure
> > in struct ib_device.
> 
> Then don't blindly cast it backwards!  Fix that up, an ib device should have
> access to the dma structures that the PCI device it depends on has.
> If not, you need to set that up properly in the IB core, don't mess with the
> driver core for this at all.
> 
I replied with pseudo code in previous reply to Bart to bring back dma_device member in the ib_device.
dma_device member was already present in near past of few weeks.
It should be able to work using it without performance impact and without touching driver core layer like in this patch.

[toc] | [prev] | [next] | [standalone]

#1593954 — RE: [PATCH 1/2] device: Stop requiring that struct device is embedded in struct pci_dev

Hi Bart,

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Bart Van Assche
> Sent: Monday, March 6, 2017 11:13 PM
> To: Parav Pandit <parav@mellanox.com>; gregkh@linuxfoundation.org
> Cc: linux-kernel@vger.kernel.org; linux-rdma@vger.kernel.org;
> sebott@linux.vnet.ibm.com; linux@armlinux.org.uk; hpa@zytor.com;
> mingo@redhat.com; dwmw2@infradead.org; bhelgaas@google.com;
> dledford@redhat.com; benh@kernel.crashing.org
> Subject: Re: [PATCH 1/2] device: Stop requiring that struct device is
> embedded in struct pci_dev
> 
> On Tue, 2017-03-07 at 05:08 +0000, Parav Pandit wrote:
> > I replied with pseudo code in previous reply to Bart to bring back
> dma_device member in the ib_device.
> > dma_device member was already present in near past of few weeks.
> > It should be able to work using it without performance impact and without
> touching driver core layer like in this patch.
> 
> That's confusing and was a source of bugs and inconsistencies. We do not
> want two device structures in struct ib_device (struct device dev and struct
> device *dma_device).

device dev represents, dev structure of the ib_device.
While dma_device is for the actual device as you know.

If you look at net_device, 
It has device dev.
vendor drivers store pci_dev pointer and access device of pci_dev etc.
Every net_device driver has to do that.

Ib_device simplifies that work for ib stack by storing dma_device.
I think this is less confusing.

> 
> Bart.N     r  y   b X  ǧv ^ )޺{.n +    {  ٚ {ay ʇڙ ,j   f   h   z  w       j:+v   w j m         zZ+
> ݢj"  ! i

[toc] | [prev] | [next] | [standalone]

#1594500 — RE: [PATCH 1/2] device: Stop requiring that struct device is embedded in struct pci_dev

> -----Original Message-----
> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, March 7, 2017 11:14 AM
> To: Bart Van Assche <Bart.VanAssche@sandisk.com>
> Cc: linux-kernel@vger.kernel.org; linux-rdma@vger.kernel.org; Parav Pandit
> <parav@mellanox.com>; sebott@linux.vnet.ibm.com;
> linux@armlinux.org.uk; hpa@zytor.com; mingo@redhat.com;
> dwmw2@infradead.org; bhelgaas@google.com; dledford@redhat.com;
> benh@kernel.crashing.org
> Subject: Re: [PATCH 1/2] device: Stop requiring that struct device is
> embedded in struct pci_dev
> 
> 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
> :)
> 

Well what I compared is:
netdev has struct device and it also has underlying pci_dev based device.
ibdev has struct device and it also has underlying pci_dev based device.
So let us try to treat them in same way wherever possible and keep setup needed in ib drivers.

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

DMA mapping and allocation is done in different layer for its own reason unrelated to this change.
If rxe, qib, hfi1 point to right dma_device, can't we remove the ib_dma_*()?

[toc] | [prev] | [next] | [standalone]

#1594571

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

[toc] | [prev] | [next] | [standalone]

#1594824

On Tue, 2017-03-07 at 05:52 +0100, Greg Kroah-Hartman wrote:
> On Mon, Mar 06, 2017 at 04:35:48PM -0800, Bart Van Assche wrote:
> > The dma mapping operations of several architectures and also of
> > several I/O MMU implementations need to translate a struct
> > device pointer into a struct pci_dev pointer. This translation
> > is performed by to_pci_dev(). That macro assumes that struct
> > device is embedded in struct pci_dev. However, that is not the
> > case for the device structure in struct ib_device.
> 
> Then don't blindly cast it backwards!  Fix that up, an ib device should
> have access to the dma structures that the PCI device it depends on has.
> If not, you need to set that up properly in the IB core, don't mess with
> the driver core for this at all.
> 
> 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 :)

Right, in his case, Bart, you can either pass the the struct device for
use for DMA to the ib devices, which is easy but a bit gross, or have
the ib core provide a set of dma_ops for the ib_device that are
"wrappers" calling back to the "parent" device.

Any struct device whose dma_ops haven't been setup by the architecture
core cannot be used for DMA as-is without such "reflector" dma_ops
provided by the creator of that struct device.

The architecture core only knows about some "directly" attached things
like PCI, some cases of platform devices etc... and has no way of
setting things up for subsystem specific thigns like ib_device.

Thus the subsystem must take care of it and provide its own dma_ops
that "reflect" the calls to the original parent device that was setup
by the architecture.

Cheers,
Ben.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web