2021-02-06 14:52:01

by Greg Kroah-Hartman

Subject: Re: [char-misc-next 6/6] mei: bus: add client dma interface

On Sat, Feb 06, 2021 at 04:43:25PM +0200, Tomas Winkler wrote:
> From: Alexander Usyskin <[email protected]>
>
> Expose the client dma mapping via mei client
> bus interface.
>
> Signed-off-by: Alexander Usyskin <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> ---
> drivers/misc/mei/bus.c | 46 ++++++++++++++++++++++++++++++++++++++
> drivers/misc/mei/hw.h | 5 +++++
> include/linux/mei_cl_bus.h | 3 +++
> 3 files changed, 54 insertions(+)
>
> diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
> index 34fb5e541fe5..d430710a5fe5 100644
> --- a/drivers/misc/mei/bus.c
> +++ b/drivers/misc/mei/bus.c
> @@ -636,6 +636,52 @@ static void mei_cl_bus_vtag_free(struct mei_cl_device *cldev)
> kfree(cl_vtag);
> }
>
> +void *mei_cldev_dma_map(struct mei_cl_device *cldev, u8 buffer_id, size_t size)
> +{
> + struct mei_device *bus;
> + struct mei_cl *cl;
> + int ret;
> +
> + if (!cldev || !buffer_id || !size)
> + return ERR_PTR(-EINVAL);
> +
> + if (!IS_ALIGNED(size, MEI_FW_PAGE_SIZE)) {
> + dev_err(&cldev->dev, "Map size should be aligned to %lu\n",
> + MEI_FW_PAGE_SIZE);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + cl = cldev->cl;
> + bus = cldev->bus;
> +
> + mutex_lock(&bus->device_lock);
> + ret = mei_cl_dma_alloc_and_map(cl, NULL, buffer_id, size);
> + mutex_unlock(&bus->device_lock);
> + if (ret)
> + return ERR_PTR(ret);
> + return cl->dma.vaddr;
> +}
> +EXPORT_SYMBOL_GPL(mei_cldev_dma_map);

Why are you exporting symbols without a user of them?

That's a sure way to get them removed by someone right after this lands
in the tree :)

Please only add infrastructure for when you have a real user.

thanks,

greg k-h

2021-02-06 15:09:24

by Winkler, Tomas

Subject: RE: [char-misc-next 6/6] mei: bus: add client dma interface

> On Sat, Feb 06, 2021 at 04:43:25PM +0200, Tomas Winkler wrote:
> > From: Alexander Usyskin <[email protected]>
> >
> > Expose the client dma mapping via mei client bus interface.
> >
> > Signed-off-by: Alexander Usyskin <[email protected]>
> > Signed-off-by: Tomas Winkler <[email protected]>
> > ---
> > drivers/misc/mei/bus.c | 46
> ++++++++++++++++++++++++++++++++++++++
> > drivers/misc/mei/hw.h | 5 +++++
> > include/linux/mei_cl_bus.h | 3 +++
> > 3 files changed, 54 insertions(+)
> >
> > diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c index
> > 34fb5e541fe5..d430710a5fe5 100644
> > --- a/drivers/misc/mei/bus.c
> > +++ b/drivers/misc/mei/bus.c
> > @@ -636,6 +636,52 @@ static void mei_cl_bus_vtag_free(struct
> mei_cl_device *cldev)
> > kfree(cl_vtag);
> > }
> >
> > +void *mei_cldev_dma_map(struct mei_cl_device *cldev, u8 buffer_id,
> > +size_t size) {
> > + struct mei_device *bus;
> > + struct mei_cl *cl;
> > + int ret;
> > +
> > + if (!cldev || !buffer_id || !size)
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (!IS_ALIGNED(size, MEI_FW_PAGE_SIZE)) {
> > + dev_err(&cldev->dev, "Map size should be aligned to %lu\n",
> > + MEI_FW_PAGE_SIZE);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + cl = cldev->cl;
> > + bus = cldev->bus;
> > +
> > + mutex_lock(&bus->device_lock);
> > + ret = mei_cl_dma_alloc_and_map(cl, NULL, buffer_id, size);
> > + mutex_unlock(&bus->device_lock);
> > + if (ret)
> > + return ERR_PTR(ret);
> > + return cl->dma.vaddr;
> > +}
> > +EXPORT_SYMBOL_GPL(mei_cldev_dma_map);
>
> Why are you exporting symbols without a user of them?
>
> That's a sure way to get them removed by someone right after this lands in
> the tree :)
>
> Please only add infrastructure for when you have a real user.
You are right, the user is coming from wifi tree. We need merge before they do.
If you prefer we can merge that all from the wifi tree.


Thanks
Tomas

2021-02-06 16:49:51

by Greg Kroah-Hartman

Subject: Re: [char-misc-next 6/6] mei: bus: add client dma interface

On Sat, Feb 06, 2021 at 03:04:34PM +0000, Winkler, Tomas wrote:
> > On Sat, Feb 06, 2021 at 04:43:25PM +0200, Tomas Winkler wrote:
> > > From: Alexander Usyskin <[email protected]>
> > >
> > > Expose the client dma mapping via mei client bus interface.
> > >
> > > Signed-off-by: Alexander Usyskin <[email protected]>
> > > Signed-off-by: Tomas Winkler <[email protected]>
> > > ---
> > > drivers/misc/mei/bus.c | 46
> > ++++++++++++++++++++++++++++++++++++++
> > > drivers/misc/mei/hw.h | 5 +++++
> > > include/linux/mei_cl_bus.h | 3 +++
> > > 3 files changed, 54 insertions(+)
> > >
> > > diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c index
> > > 34fb5e541fe5..d430710a5fe5 100644
> > > --- a/drivers/misc/mei/bus.c
> > > +++ b/drivers/misc/mei/bus.c
> > > @@ -636,6 +636,52 @@ static void mei_cl_bus_vtag_free(struct
> > mei_cl_device *cldev)
> > > kfree(cl_vtag);
> > > }
> > >
> > > +void *mei_cldev_dma_map(struct mei_cl_device *cldev, u8 buffer_id,
> > > +size_t size) {
> > > + struct mei_device *bus;
> > > + struct mei_cl *cl;
> > > + int ret;
> > > +
> > > + if (!cldev || !buffer_id || !size)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + if (!IS_ALIGNED(size, MEI_FW_PAGE_SIZE)) {
> > > + dev_err(&cldev->dev, "Map size should be aligned to %lu\n",
> > > + MEI_FW_PAGE_SIZE);
> > > + return ERR_PTR(-EINVAL);
> > > + }
> > > +
> > > + cl = cldev->cl;
> > > + bus = cldev->bus;
> > > +
> > > + mutex_lock(&bus->device_lock);
> > > + ret = mei_cl_dma_alloc_and_map(cl, NULL, buffer_id, size);
> > > + mutex_unlock(&bus->device_lock);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > + return cl->dma.vaddr;
> > > +}
> > > +EXPORT_SYMBOL_GPL(mei_cldev_dma_map);
> >
> > Why are you exporting symbols without a user of them?
> >
> > That's a sure way to get them removed by someone right after this lands in
> > the tree :)
> >
> > Please only add infrastructure for when you have a real user.
> You are right, the user is coming from wifi tree. We need merge before they do.
> If you prefer we can merge that all from the wifi tree.

That wasn't documented well at all in your changelog comment :(

It can go through whatever tree needs it.

thanks,

greg k-h

2021-02-07 14:11:12

by Winkler, Tomas

Subject: RE: [char-misc-next 6/6] mei: bus: add client dma interface

>
> On Sat, Feb 06, 2021 at 03:04:34PM +0000, Winkler, Tomas wrote:
> > > On Sat, Feb 06, 2021 at 04:43:25PM +0200, Tomas Winkler wrote:
> > > > From: Alexander Usyskin <[email protected]>
> > > >
> > > > Expose the client dma mapping via mei client bus interface.
> > > >
> > > > Signed-off-by: Alexander Usyskin <[email protected]>
> > > > Signed-off-by: Tomas Winkler <[email protected]>
> > > > ---
> > > > drivers/misc/mei/bus.c | 46
> > > ++++++++++++++++++++++++++++++++++++++
> > > > drivers/misc/mei/hw.h | 5 +++++
> > > > include/linux/mei_cl_bus.h | 3 +++
> > > > 3 files changed, 54 insertions(+)
> > > >
> > > > diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c index
> > > > 34fb5e541fe5..d430710a5fe5 100644
> > > > --- a/drivers/misc/mei/bus.c
> > > > +++ b/drivers/misc/mei/bus.c
> > > > @@ -636,6 +636,52 @@ static void mei_cl_bus_vtag_free(struct
> > > mei_cl_device *cldev)
> > > > kfree(cl_vtag);
> > > > }
> > > >
> > > > +void *mei_cldev_dma_map(struct mei_cl_device *cldev, u8
> > > > +buffer_id, size_t size) {
> > > > + struct mei_device *bus;
> > > > + struct mei_cl *cl;
> > > > + int ret;
> > > > +
> > > > + if (!cldev || !buffer_id || !size)
> > > > + return ERR_PTR(-EINVAL);
> > > > +
> > > > + if (!IS_ALIGNED(size, MEI_FW_PAGE_SIZE)) {
> > > > + dev_err(&cldev->dev, "Map size should be aligned to %lu\n",
> > > > + MEI_FW_PAGE_SIZE);
> > > > + return ERR_PTR(-EINVAL);
> > > > + }
> > > > +
> > > > + cl = cldev->cl;
> > > > + bus = cldev->bus;
> > > > +
> > > > + mutex_lock(&bus->device_lock);
> > > > + ret = mei_cl_dma_alloc_and_map(cl, NULL, buffer_id, size);
> > > > + mutex_unlock(&bus->device_lock);
> > > > + if (ret)
> > > > + return ERR_PTR(ret);
> > > > + return cl->dma.vaddr;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(mei_cldev_dma_map);
> > >
> > > Why are you exporting symbols without a user of them?
> > >
> > > That's a sure way to get them removed by someone right after this
> > > lands in the tree :)
> > >
> > > Please only add infrastructure for when you have a real user.
> > You are right, the user is coming from wifi tree. We need merge before
> they do.
> > If you prefer we can merge that all from the wifi tree.
>
> That wasn't documented well at all in your changelog comment :(
Note taken.
> It can go through whatever tree needs it.
If possible please merge our changes already here, and let the wifi guys to finish their task in their tree, in their own time.
Thanks
Tomas

2021-02-07 14:35:16

by Greg Kroah-Hartman

Subject: Re: [char-misc-next 6/6] mei: bus: add client dma interface

On Sun, Feb 07, 2021 at 02:03:11PM +0000, Winkler, Tomas wrote:
> >
> > On Sat, Feb 06, 2021 at 03:04:34PM +0000, Winkler, Tomas wrote:
> > > > On Sat, Feb 06, 2021 at 04:43:25PM +0200, Tomas Winkler wrote:
> > > > > From: Alexander Usyskin <[email protected]>
> > > > >
> > > > > Expose the client dma mapping via mei client bus interface.
> > > > >
> > > > > Signed-off-by: Alexander Usyskin <[email protected]>
> > > > > Signed-off-by: Tomas Winkler <[email protected]>
> > > > > ---
> > > > > drivers/misc/mei/bus.c | 46
> > > > ++++++++++++++++++++++++++++++++++++++
> > > > > drivers/misc/mei/hw.h | 5 +++++
> > > > > include/linux/mei_cl_bus.h | 3 +++
> > > > > 3 files changed, 54 insertions(+)
> > > > >
> > > > > diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c index
> > > > > 34fb5e541fe5..d430710a5fe5 100644
> > > > > --- a/drivers/misc/mei/bus.c
> > > > > +++ b/drivers/misc/mei/bus.c
> > > > > @@ -636,6 +636,52 @@ static void mei_cl_bus_vtag_free(struct
> > > > mei_cl_device *cldev)
> > > > > kfree(cl_vtag);
> > > > > }
> > > > >
> > > > > +void *mei_cldev_dma_map(struct mei_cl_device *cldev, u8
> > > > > +buffer_id, size_t size) {
> > > > > + struct mei_device *bus;
> > > > > + struct mei_cl *cl;
> > > > > + int ret;
> > > > > +
> > > > > + if (!cldev || !buffer_id || !size)
> > > > > + return ERR_PTR(-EINVAL);
> > > > > +
> > > > > + if (!IS_ALIGNED(size, MEI_FW_PAGE_SIZE)) {
> > > > > + dev_err(&cldev->dev, "Map size should be aligned to %lu\n",
> > > > > + MEI_FW_PAGE_SIZE);
> > > > > + return ERR_PTR(-EINVAL);
> > > > > + }
> > > > > +
> > > > > + cl = cldev->cl;
> > > > > + bus = cldev->bus;
> > > > > +
> > > > > + mutex_lock(&bus->device_lock);
> > > > > + ret = mei_cl_dma_alloc_and_map(cl, NULL, buffer_id, size);
> > > > > + mutex_unlock(&bus->device_lock);
> > > > > + if (ret)
> > > > > + return ERR_PTR(ret);
> > > > > + return cl->dma.vaddr;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(mei_cldev_dma_map);
> > > >
> > > > Why are you exporting symbols without a user of them?
> > > >
> > > > That's a sure way to get them removed by someone right after this
> > > > lands in the tree :)
> > > >
> > > > Please only add infrastructure for when you have a real user.
> > > You are right, the user is coming from wifi tree. We need merge before
> > they do.
> > > If you prefer we can merge that all from the wifi tree.
> >
> > That wasn't documented well at all in your changelog comment :(
> Note taken.
> > It can go through whatever tree needs it.
> If possible please merge our changes already here, and let the wifi guys to finish their task in their tree, in their own time.

I took some of these already, patches 1 and 6 I can't take as-is, sorry.

thanks,

greg k-h

2021-02-07 19:08:07

by Grumbach, Emmanuel

Subject: RE: [char-misc-next 6/6] mei: bus: add client dma interface

Hi Greg,

> On Sun, Feb 07, 2021 at 02:03:11PM +0000, Winkler, Tomas wrote:
> > >
> > > On Sat, Feb 06, 2021 at 03:04:34PM +0000, Winkler, Tomas wrote:
> > > > > On Sat, Feb 06, 2021 at 04:43:25PM +0200, Tomas Winkler wrote:
> > > > > > From: Alexander Usyskin <[email protected]>
> > > > > >
> > > > > > Expose the client dma mapping via mei client bus interface.
> > > > > >
> > > > > > Signed-off-by: Alexander Usyskin <[email protected]>
> > > > > > Signed-off-by: Tomas Winkler <[email protected]>
> > > > > > ---
> > > > > > drivers/misc/mei/bus.c | 46
> > > > > ++++++++++++++++++++++++++++++++++++++
> > > > > > drivers/misc/mei/hw.h | 5 +++++
> > > > > > include/linux/mei_cl_bus.h | 3 +++
> > > > > > 3 files changed, 54 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
> > > > > > index
> > > > > > 34fb5e541fe5..d430710a5fe5 100644
> > > > > > --- a/drivers/misc/mei/bus.c
> > > > > > +++ b/drivers/misc/mei/bus.c
> > > > > > @@ -636,6 +636,52 @@ static void mei_cl_bus_vtag_free(struct
> > > > > mei_cl_device *cldev)
> > > > > > kfree(cl_vtag);
> > > > > > }
> > > > > >
> > > > > > +void *mei_cldev_dma_map(struct mei_cl_device *cldev, u8
> > > > > > +buffer_id, size_t size) {
> > > > > > + struct mei_device *bus;
> > > > > > + struct mei_cl *cl;
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (!cldev || !buffer_id || !size)
> > > > > > + return ERR_PTR(-EINVAL);
> > > > > > +
> > > > > > + if (!IS_ALIGNED(size, MEI_FW_PAGE_SIZE)) {
> > > > > > + dev_err(&cldev->dev, "Map size should be aligned to
> %lu\n",
> > > > > > + MEI_FW_PAGE_SIZE);
> > > > > > + return ERR_PTR(-EINVAL);
> > > > > > + }
> > > > > > +
> > > > > > + cl = cldev->cl;
> > > > > > + bus = cldev->bus;
> > > > > > +
> > > > > > + mutex_lock(&bus->device_lock);
> > > > > > + ret = mei_cl_dma_alloc_and_map(cl, NULL, buffer_id, size);
> > > > > > + mutex_unlock(&bus->device_lock);
> > > > > > + if (ret)
> > > > > > + return ERR_PTR(ret);
> > > > > > + return cl->dma.vaddr;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(mei_cldev_dma_map);
> > > > >
> > > > > Why are you exporting symbols without a user of them?
> > > > >
> > > > > That's a sure way to get them removed by someone right after
> > > > > this lands in the tree :)
> > > > >
> > > > > Please only add infrastructure for when you have a real user.
> > > > You are right, the user is coming from wifi tree. We need merge
> > > > before
> > > they do.
> > > > If you prefer we can merge that all from the wifi tree.
> > >
> > > That wasn't documented well at all in your changelog comment :(
> > Note taken.
> > > It can go through whatever tree needs it.
> > If possible please merge our changes already here, and let the wifi guys to
> finish their task in their tree, in their own time.
>
> I took some of these already, patches 1 and 6 I can't take as-is, sorry.

As Tomas said, 6 is needed by iwlwifi. I understand you want it to be routed through iwlwifi-next.git. I added Kalle so that he gets a heads up.
1 is not related to iwlwifi.