Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-15 Thread Lu Baolu

Hi Greg,

On 2021/12/13 8:50, Lu Baolu wrote:

On 12/10/21 9:23 AM, Lu Baolu wrote:

Hi Greg, Jason and Christoph,

On 12/9/21 9:20 AM, Lu Baolu wrote:

On 12/7/21 9:16 PM, Jason Gunthorpe wrote:

On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote:

On 12/6/21 11:06 PM, Jason Gunthorpe wrote:

On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote:
I really hate the amount of boilerplate code that having this in 
each

bus type causes.

+1

I liked the first version of this series better with the code near
really_probe().

Can we go back to that with some device_configure_dma() wrapper
condtionally called by really_probe as we discussed?


[...]



diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 68ea1f949daa..68ca5a579eb1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -538,6 +538,32 @@ static int call_driver_probe(struct device *dev, 
struct device_driver *drv)

 return ret;
  }

+static int device_dma_configure(struct device *dev, struct 
device_driver *drv)

+{
+   int ret;
+
+   if (!dev->bus->dma_configure)
+   return 0;
+
+   ret = dev->bus->dma_configure(dev);
+   if (ret)
+   return ret;
+
+   if (!drv->suppress_auto_claim_dma_owner)
+   ret = iommu_device_set_dma_owner(dev, 
DMA_OWNER_DMA_API, NULL);

+
+   return ret;
+}
+
+static void device_dma_cleanup(struct device *dev, struct 
device_driver *drv)

+{
+   if (!dev->bus->dma_configure)
+   return;
+
+   if (!drv->suppress_auto_claim_dma_owner)
+   iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API);
+}
+
  static int really_probe(struct device *dev, struct device_driver *drv)
  {
 bool test_remove = 
IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
@@ -574,11 +600,8 @@ static int really_probe(struct device *dev, 
struct device_driver *drv)

 if (ret)
 goto pinctrl_bind_failed;

-   if (dev->bus->dma_configure) {
-   ret = dev->bus->dma_configure(dev);
-   if (ret)
-   goto probe_failed;
-   }
+   if (device_dma_configure(dev, drv))
+   goto pinctrl_bind_failed;

 ret = driver_sysfs_add(dev);
 if (ret) {
@@ -660,6 +683,8 @@ static int really_probe(struct device *dev, 
struct device_driver *drv)

 if (dev->bus)
 
blocking_notifier_call_chain(>bus->p->bus_notifier,


BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
+
+   device_dma_cleanup(dev, drv);
  pinctrl_bind_failed:
 device_links_no_driver(dev);
 devres_release_all(dev);
@@ -1204,6 +1229,7 @@ static void __device_release_driver(struct 
device *dev, struct device *parent)

 else if (drv->remove)
 drv->remove(dev);

+   device_dma_cleanup(dev, drv);
 device_links_driver_cleanup(dev);

 devres_release_all(dev);
diff --git a/include/linux/device/driver.h 
b/include/linux/device/driver.h

index a498ebcf4993..374a3c2cc10d 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -100,6 +100,7 @@ struct device_driver {
 const char  *mod_name;  /* used for built-in 
modules */


 bool suppress_bind_attrs;   /* disables bind/unbind via 
sysfs */

+   bool suppress_auto_claim_dma_owner;
 enum probe_type probe_type;

 const struct of_device_id   *of_match_table;


Does this work for you? Can I work towards this in the next version?


A kindly ping ... Is this heading the right direction? I need your
advice to move ahead. :-)


Can I do it like this? :-)

Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-14 Thread Christoph Hellwig
This approach looks much better to me.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-13 Thread Jason Gunthorpe via iommu
On Mon, Dec 13, 2021 at 08:50:05AM +0800, Lu Baolu wrote:

> > Does this work for you? Can I work towards this in the next version?
> 
> A kindly ping ... Is this heading the right direction? I need your
> advice to move ahead. :-)

I prefer this to all the duplicated code in the v3 series.. Given it
touches almost every bus type that implements the dma_configure it
seems appropriate.

Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-12 Thread Lu Baolu

On 12/10/21 9:23 AM, Lu Baolu wrote:

Hi Greg, Jason and Christoph,

On 12/9/21 9:20 AM, Lu Baolu wrote:

On 12/7/21 9:16 PM, Jason Gunthorpe wrote:

On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote:

On 12/6/21 11:06 PM, Jason Gunthorpe wrote:

On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote:

I really hate the amount of boilerplate code that having this in each
bus type causes.

+1

I liked the first version of this series better with the code near
really_probe().

Can we go back to that with some device_configure_dma() wrapper
condtionally called by really_probe as we discussed?


[...]



diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 68ea1f949daa..68ca5a579eb1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -538,6 +538,32 @@ static int call_driver_probe(struct device *dev, 
struct device_driver *drv)

 return ret;
  }

+static int device_dma_configure(struct device *dev, struct 
device_driver *drv)

+{
+   int ret;
+
+   if (!dev->bus->dma_configure)
+   return 0;
+
+   ret = dev->bus->dma_configure(dev);
+   if (ret)
+   return ret;
+
+   if (!drv->suppress_auto_claim_dma_owner)
+   ret = iommu_device_set_dma_owner(dev, 
DMA_OWNER_DMA_API, NULL);

+
+   return ret;
+}
+
+static void device_dma_cleanup(struct device *dev, struct 
device_driver *drv)

+{
+   if (!dev->bus->dma_configure)
+   return;
+
+   if (!drv->suppress_auto_claim_dma_owner)
+   iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API);
+}
+
  static int really_probe(struct device *dev, struct device_driver *drv)
  {
 bool test_remove = 
IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
@@ -574,11 +600,8 @@ static int really_probe(struct device *dev, 
struct device_driver *drv)

 if (ret)
 goto pinctrl_bind_failed;

-   if (dev->bus->dma_configure) {
-   ret = dev->bus->dma_configure(dev);
-   if (ret)
-   goto probe_failed;
-   }
+   if (device_dma_configure(dev, drv))
+   goto pinctrl_bind_failed;

 ret = driver_sysfs_add(dev);
 if (ret) {
@@ -660,6 +683,8 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)

 if (dev->bus)
 blocking_notifier_call_chain(>bus->p->bus_notifier,

BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
+
+   device_dma_cleanup(dev, drv);
  pinctrl_bind_failed:
 device_links_no_driver(dev);
 devres_release_all(dev);
@@ -1204,6 +1229,7 @@ static void __device_release_driver(struct 
device *dev, struct device *parent)

 else if (drv->remove)
 drv->remove(dev);

+   device_dma_cleanup(dev, drv);
 device_links_driver_cleanup(dev);

 devres_release_all(dev);
diff --git a/include/linux/device/driver.h 
b/include/linux/device/driver.h

index a498ebcf4993..374a3c2cc10d 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -100,6 +100,7 @@ struct device_driver {
 const char  *mod_name;  /* used for built-in 
modules */


 bool suppress_bind_attrs;   /* disables bind/unbind via 
sysfs */

+   bool suppress_auto_claim_dma_owner;
 enum probe_type probe_type;

 const struct of_device_id   *of_match_table;


Does this work for you? Can I work towards this in the next version?


A kindly ping ... Is this heading the right direction? I need your
advice to move ahead. :-)

Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-09 Thread Lu Baolu

Hi Greg, Jason and Christoph,

On 12/9/21 9:20 AM, Lu Baolu wrote:

On 12/7/21 9:16 PM, Jason Gunthorpe wrote:

On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote:

On 12/6/21 11:06 PM, Jason Gunthorpe wrote:

On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote:

I really hate the amount of boilerplate code that having this in each
bus type causes.

+1

I liked the first version of this series better with the code near
really_probe().

Can we go back to that with some device_configure_dma() wrapper
condtionally called by really_probe as we discussed?


[...]



diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 68ea1f949daa..68ca5a579eb1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -538,6 +538,32 @@ static int call_driver_probe(struct device *dev, 
struct device_driver *drv)

     return ret;
  }

+static int device_dma_configure(struct device *dev, struct 
device_driver *drv)

+{
+   int ret;
+
+   if (!dev->bus->dma_configure)
+   return 0;
+
+   ret = dev->bus->dma_configure(dev);
+   if (ret)
+   return ret;
+
+   if (!drv->suppress_auto_claim_dma_owner)
+   ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, 
NULL);

+
+   return ret;
+}
+
+static void device_dma_cleanup(struct device *dev, struct device_driver 
*drv)

+{
+   if (!dev->bus->dma_configure)
+   return;
+
+   if (!drv->suppress_auto_claim_dma_owner)
+   iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API, 
NULL);

+}
+
  static int really_probe(struct device *dev, struct device_driver *drv)
  {
     bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
@@ -574,11 +600,8 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)

     if (ret)
     goto pinctrl_bind_failed;

-   if (dev->bus->dma_configure) {
-   ret = dev->bus->dma_configure(dev);
-   if (ret)
-   goto probe_failed;
-   }
+   if (device_dma_configure(dev, drv))
+   goto pinctrl_bind_failed;

     ret = driver_sysfs_add(dev);
     if (ret) {
@@ -660,6 +683,8 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)

     if (dev->bus)
     blocking_notifier_call_chain(>bus->p->bus_notifier,

BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
+
+   device_dma_cleanup(dev, drv);
  pinctrl_bind_failed:
     device_links_no_driver(dev);
     devres_release_all(dev);
@@ -1204,6 +1229,7 @@ static void __device_release_driver(struct device 
*dev, struct device *parent)

     else if (drv->remove)
     drv->remove(dev);

+   device_dma_cleanup(dev, drv);
     device_links_driver_cleanup(dev);

     devres_release_all(dev);
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index a498ebcf4993..374a3c2cc10d 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -100,6 +100,7 @@ struct device_driver {
     const char  *mod_name;  /* used for built-in 
modules */


     bool suppress_bind_attrs;   /* disables bind/unbind via 
sysfs */

+   bool suppress_auto_claim_dma_owner;
     enum probe_type probe_type;

     const struct of_device_id   *of_match_table;


Does this work for you? Can I work towards this in the next version?

Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-08 Thread Lu Baolu

On 12/7/21 9:16 PM, Jason Gunthorpe wrote:

On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote:

On 12/6/21 11:06 PM, Jason Gunthorpe wrote:

On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote:

I really hate the amount of boilerplate code that having this in each
bus type causes.

+1

I liked the first version of this series better with the code near
really_probe().

Can we go back to that with some device_configure_dma() wrapper
condtionally called by really_probe as we discussed?



Are you talking about below change?

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 68ea1f949daa..368f9e530515 100644
+++ b/drivers/base/dd.c
@@ -577,7 +577,13 @@ static int really_probe(struct device *dev, struct
device_driver *drv)
if (dev->bus->dma_configure) {
ret = dev->bus->dma_configure(dev);
if (ret)
-   goto probe_failed;
+   goto pinctrl_bind_failed;
+
+   if (!drv->no_kernel_dma) {
+   ret = iommu_device_set_dma_owner(dev, 
DMA_OWNER_DMA_API, NULL);
+   if (ret)
+   goto pinctrl_bind_failed;
+}
}


Yes, the suggestion was to put everything that 'if' inside a function
and then of course a matching undo function.


Followed your suggestion, I refactored the change like below:

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 68ea1f949daa..68ca5a579eb1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -538,6 +538,32 @@ static int call_driver_probe(struct device *dev, 
struct device_driver *drv)

return ret;
 }

+static int device_dma_configure(struct device *dev, struct 
device_driver *drv)

+{
+   int ret;
+
+   if (!dev->bus->dma_configure)
+   return 0;
+
+   ret = dev->bus->dma_configure(dev);
+   if (ret)
+   return ret;
+
+   if (!drv->suppress_auto_claim_dma_owner)
+   ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, 
NULL);

+
+   return ret;
+}
+
+static void device_dma_cleanup(struct device *dev, struct device_driver 
*drv)

+{
+   if (!dev->bus->dma_configure)
+   return;
+
+   if (!drv->suppress_auto_claim_dma_owner)
+   iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API, 
NULL);

+}
+
 static int really_probe(struct device *dev, struct device_driver *drv)
 {
bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
@@ -574,11 +600,8 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)

if (ret)
goto pinctrl_bind_failed;

-   if (dev->bus->dma_configure) {
-   ret = dev->bus->dma_configure(dev);
-   if (ret)
-   goto probe_failed;
-   }
+   if (device_dma_configure(dev, drv))
+   goto pinctrl_bind_failed;

ret = driver_sysfs_add(dev);
if (ret) {
@@ -660,6 +683,8 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)

if (dev->bus)
blocking_notifier_call_chain(>bus->p->bus_notifier,

BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
+
+   device_dma_cleanup(dev, drv);
 pinctrl_bind_failed:
device_links_no_driver(dev);
devres_release_all(dev);
@@ -1204,6 +1229,7 @@ static void __device_release_driver(struct device 
*dev, struct device *parent)

else if (drv->remove)
drv->remove(dev);

+   device_dma_cleanup(dev, drv);
device_links_driver_cleanup(dev);

devres_release_all(dev);
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index a498ebcf4993..374a3c2cc10d 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -100,6 +100,7 @@ struct device_driver {
const char  *mod_name;  /* used for built-in 
modules */


bool suppress_bind_attrs;   /* disables bind/unbind via 
sysfs */

+   bool suppress_auto_claim_dma_owner;
enum probe_type probe_type;

const struct of_device_id   *of_match_table;

Further suggestions?

Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-07 Thread Jason Gunthorpe via iommu
On Tue, Dec 07, 2021 at 05:25:04AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 07, 2021 at 09:16:27AM -0400, Jason Gunthorpe wrote:
> > Yes, the suggestion was to put everything that 'if' inside a function
> > and then of course a matching undo function.
> 
> Can't we simplify things even more?  Do away with the DMA API owner
> entirely, and instead in iommu_group_set_dma_owner iterate over all
> devices in a group and check that they all have the no_dma_api flag
> set (plus a similar check on group join).  With that most of the
> boilerplate code goes away entirely in favor of a little more work at
> iommu_group_set_dma_owner time.

Robin suggested something like this already.

The locking doesn't work out, we can't nest device_lock()'s safely
without ABBA deadlocks, and can't touch the dev->driver without the
device_lock.

Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-07 Thread Christoph Hellwig
On Tue, Dec 07, 2021 at 09:16:27AM -0400, Jason Gunthorpe wrote:
> Yes, the suggestion was to put everything that 'if' inside a function
> and then of course a matching undo function.

Can't we simplify things even more?  Do away with the DMA API owner
entirely, and instead in iommu_group_set_dma_owner iterate over all
devices in a group and check that they all have the no_dma_api flag
set (plus a similar check on group join).  With that most of the
boilerplate code goes away entirely in favor of a little more work at
iommu_group_set_dma_owner time.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-07 Thread Jason Gunthorpe via iommu
On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote:
> On 12/6/21 11:06 PM, Jason Gunthorpe wrote:
> > On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote:
> > > I really hate the amount of boilerplate code that having this in each
> > > bus type causes.
> > +1
> > 
> > I liked the first version of this series better with the code near
> > really_probe().
> > 
> > Can we go back to that with some device_configure_dma() wrapper
> > condtionally called by really_probe as we discussed?
> > 
> 
> Are you talking about below change?
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 68ea1f949daa..368f9e530515 100644
> +++ b/drivers/base/dd.c
> @@ -577,7 +577,13 @@ static int really_probe(struct device *dev, struct
> device_driver *drv)
>   if (dev->bus->dma_configure) {
>   ret = dev->bus->dma_configure(dev);
>   if (ret)
> - goto probe_failed;
> + goto pinctrl_bind_failed;
> +
> + if (!drv->no_kernel_dma) {
> + ret = iommu_device_set_dma_owner(dev, 
> DMA_OWNER_DMA_API, NULL);
> + if (ret)
> + goto pinctrl_bind_failed;
> +}
>   }

Yes, the suggestion was to put everything that 'if' inside a function
and then of course a matching undo function.

Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-06 Thread Lu Baolu

On 12/6/21 11:06 PM, Jason Gunthorpe wrote:

On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote:

I really hate the amount of boilerplate code that having this in each
bus type causes.

+1

I liked the first version of this series better with the code near
really_probe().

Can we go back to that with some device_configure_dma() wrapper
condtionally called by really_probe as we discussed?



Are you talking about below change?

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 68ea1f949daa..368f9e530515 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -577,7 +577,13 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)

if (dev->bus->dma_configure) {
ret = dev->bus->dma_configure(dev);
if (ret)
-   goto probe_failed;
+   goto pinctrl_bind_failed;
+
+   if (!drv->no_kernel_dma) {
+   ret = iommu_device_set_dma_owner(dev, 
DMA_OWNER_DMA_API, NULL);
+   if (ret)
+   goto pinctrl_bind_failed;
+}
}

ret = driver_sysfs_add(dev);
@@ -660,6 +666,9 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)

if (dev->bus)
blocking_notifier_call_chain(>bus->p->bus_notifier,
 BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
+
+   if (dev->bus->dma_configure && !drv->no_kernel_dma)
+   iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API);
 pinctrl_bind_failed:
device_links_no_driver(dev);
devres_release_all(dev);
@@ -1204,6 +1213,9 @@ static void __device_release_driver(struct device 
*dev, struct device *parent)

else if (drv->remove)
drv->remove(dev);

+   if (dev->bus->dma_configure && !drv->no_kernel_dma)
+   iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API);
+
device_links_driver_cleanup(dev);

devres_release_all(dev);
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index a498ebcf4993..2cf7b757b28e 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -100,6 +100,7 @@ struct device_driver {
const char  *mod_name;  /* used for built-in modules */

bool suppress_bind_attrs;   /* disables bind/unbind via sysfs */
+   bool no_kernel_dma;
enum probe_type probe_type;

const struct of_device_id   *of_match_table;

Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-06 Thread Jason Gunthorpe via iommu
On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote:
> I really hate the amount of boilerplate code that having this in each
> bus type causes.

+1 

I liked the first version of this series better with the code near
really_probe().

Can we go back to that with some device_configure_dma() wrapper
condtionally called by really_probe as we discussed?

> Between that and the suggestion from Joerg I wonder if we could do the
> following again:
> 
>  - add new no_kernel_dma flag to struct device_driver
>  - set this flag for the various vfio drivers
>  - skip claiming the kernel dma ownership for those (or rather release
>it if the suggestion from Joerg works out)

v1 did exactly this.

Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-06 Thread Christoph Hellwig
I really hate the amount of boilerplate code that having this in each
bus type causes.

Between that and the suggestion from Joerg I wonder if we could do the
following again:

 - add new no_kernel_dma flag to struct device_driver
 - set this flag for the various vfio drivers
 - skip claiming the kernel dma ownership for those (or rather release
   it if the suggestion from Joerg works out)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-05 Thread Greg Kroah-Hartman
On Mon, Dec 06, 2021 at 09:58:49AM +0800, Lu Baolu wrote:
> Multiple platform devices may be placed in the same IOMMU group because
> they cannot be isolated from each other. These devices must either be
> entirely under kernel control or userspace control, never a mixture. This
> checks and sets DMA ownership during driver binding, and release the
> ownership during driver unbinding.
> 
> Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto
> claiming DMA_OWNER_DMA_API ownership in the binding process. For instance,
> the userspace framework drivers (vfio etc.) which need to manually claim
> DMA_OWNER_PRIVATE_DOMAIN_USER when assigning a device to userspace.
> 
> Signed-off-by: Lu Baolu 
> ---
>  include/linux/platform_device.h |  1 +
>  drivers/base/platform.c | 30 +-
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 4381c34af7e0..f3926be7582f 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -210,6 +210,7 @@ struct platform_driver {
>   struct device_driver driver;
>   const struct platform_device_id *id_table;
>   bool prevent_deferred_probe;
> + bool suppress_auto_claim_dma_owner;

We now have "prevent_" and "suppress_" as prefixes.  Why the difference?

What is wrong with "prevent_" for your new flag?

thanks,

greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v3 04/18] driver core: platform: Add driver dma ownership management

2021-12-05 Thread Lu Baolu
Multiple platform devices may be placed in the same IOMMU group because
they cannot be isolated from each other. These devices must either be
entirely under kernel control or userspace control, never a mixture. This
checks and sets DMA ownership during driver binding, and release the
ownership during driver unbinding.

Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto
claiming DMA_OWNER_DMA_API ownership in the binding process. For instance,
the userspace framework drivers (vfio etc.) which need to manually claim
DMA_OWNER_PRIVATE_DOMAIN_USER when assigning a device to userspace.

Signed-off-by: Lu Baolu 
---
 include/linux/platform_device.h |  1 +
 drivers/base/platform.c | 30 +-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 4381c34af7e0..f3926be7582f 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -210,6 +210,7 @@ struct platform_driver {
struct device_driver driver;
const struct platform_device_id *id_table;
bool prevent_deferred_probe;
+   bool suppress_auto_claim_dma_owner;
 };
 
 #define to_platform_driver(drv)(container_of((drv), struct 
platform_driver, \
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 125d708c0eb3..032b9f20b468 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "base.h"
 #include "power/power.h"
@@ -1464,6 +1465,32 @@ int firmware_dma_configure(struct device *dev)
return ret;
 }
 
+static int platform_dma_configure(struct device *dev)
+{
+   struct platform_driver *drv = to_platform_driver(dev->driver);
+   int ret;
+
+   if (!drv->suppress_auto_claim_dma_owner) {
+   ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, NULL);
+   if (ret)
+   return ret;
+   }
+
+   ret = firmware_dma_configure(dev);
+   if (ret && !drv->suppress_auto_claim_dma_owner)
+   iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API);
+
+   return ret;
+}
+
+static void platform_dma_cleanup(struct device *dev)
+{
+   struct platform_driver *drv = to_platform_driver(dev->driver);
+
+   if (!drv->suppress_auto_claim_dma_owner)
+   iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API);
+}
+
 static const struct dev_pm_ops platform_dev_pm_ops = {
SET_RUNTIME_PM_OPS(pm_generic_runtime_suspend, 
pm_generic_runtime_resume, NULL)
USE_PLATFORM_PM_SLEEP_OPS
@@ -1477,7 +1504,8 @@ struct bus_type platform_bus_type = {
.probe  = platform_probe,
.remove = platform_remove,
.shutdown   = platform_shutdown,
-   .dma_configure  = firmware_dma_configure,
+   .dma_configure  = platform_dma_configure,
+   .dma_cleanup= platform_dma_cleanup,
.pm = _dev_pm_ops,
 };
 EXPORT_SYMBOL_GPL(platform_bus_type);
-- 
2.25.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu