Hi Jason,
On 04/22/2025, Jason Gunthorpe wrote:
> On Mon, Apr 21, 2025 at 02:19:35PM -0700, William McVicker wrote:
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 1813cfd0c4bd..6d124447545c 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -1440,8 +1440,8 @@ static void platform_shutdown(struct device *_dev)
> >
> > static int platform_dma_configure(struct device *dev)
> > {
> > - struct platform_driver *drv = to_platform_driver(dev->driver);
> > struct fwnode_handle *fwnode = dev_fwnode(dev);
> > + struct platform_driver *drv;
> > enum dev_dma_attr attr;
> > int ret = 0;
> >
> > @@ -1451,8 +1451,12 @@ static int platform_dma_configure(struct device *dev)
> > attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
> > ret = acpi_dma_configure(dev, attr);
> > }
> > - /* @drv may not be valid when we're called from the IOMMU layer */
> > - if (ret || !dev->driver || drv->driver_managed_dma)
> > + /* @dev->driver may not be valid when we're called from the IOMMU layer */
> > + if (ret || !dev->driver)
> > + return ret;
> > +
> > + drv = to_platform_driver(dev->driver);
> > + if (drv->driver_managed_dma)
> > return ret;
> >
> > ret = iommu_device_use_default_domain(dev);
>
> The diagnosis looks right to me, but pedantically I think it should
> have a READ_ONCE():
>
> struct driver *drv = READ_ONCE(dev->driver);
>
> And then never touch dev->driver again in the function.
>
> Send a proper patch?
>
> Jason
Thanks for the response! Yes, that would work as well. I'll send a v2 revision
once I get it tested.
On this note, I was looking through `of_dma_configure_id()` and am also
wondering if we may hit other race conditions if the device is still being
probed and the dma properties (like the coherent dma mask) haven't been fully
populated? Just checking if the driver is bound, doesn't seem like enough to
start configuring the DMA when async probing can happen.
Thanks,
Will
