| Message ID | 8-v1-720585788a7d+811b-iommu_fwspec_p1_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOMMU related FW parsing cleanup | expand |
On Tue, Nov 28, 2023 at 08:48:04PM -0400, Jason Gunthorpe wrote: > This API was defined to formalize the access to internal iommu details on > some Tegra SOCs, but a few callers got missed. Add them. > > The helper already masks by 0xFFFF so remove this code from the callers. > > Suggested-by: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/dma/tegra186-gpc-dma.c | 8 +++----- > drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 7 ++----- > drivers/memory/tegra/tegra186.c | 12 ++++++------ > 3 files changed, 11 insertions(+), 16 deletions(-) > > diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c > index fa4d4142a68a21..88547a23825b18 100644 > --- a/drivers/dma/tegra186-gpc-dma.c > +++ b/drivers/dma/tegra186-gpc-dma.c > @@ -1348,8 +1348,8 @@ static int tegra_dma_program_sid(struct tegra_dma_channel *tdc, int stream_id) > static int tegra_dma_probe(struct platform_device *pdev) > { > const struct tegra_dma_chip_data *cdata = NULL; > - struct iommu_fwspec *iommu_spec; > - unsigned int stream_id, i; > + unsigned int i; > + u32 stream_id; > struct tegra_dma *tdma; > int ret; > > @@ -1378,12 +1378,10 @@ static int tegra_dma_probe(struct platform_device *pdev) > > tdma->dma_dev.dev = &pdev->dev; > > - iommu_spec = dev_iommu_fwspec_get(&pdev->dev); > - if (!iommu_spec) { > + if (!tegra_dev_iommu_get_stream_id(&pdev->dev, &stream_id)) { > dev_err(&pdev->dev, "Missing iommu stream-id\n"); > return -EINVAL; > } > - stream_id = iommu_spec->ids[0] & 0xffff; > > ret = device_property_read_u32(&pdev->dev, "dma-channel-mask", > &tdma->chan_mask); > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c > index e7e8fdf3adab7a..b40fd1dbb21617 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c > @@ -28,16 +28,13 @@ static void > gp10b_ltc_init(struct nvkm_ltc *ltc) > { > struct nvkm_device *device = ltc->subdev.device; > - struct iommu_fwspec *spec; > + u32 sid; > > nvkm_wr32(device, 0x17e27c, ltc->ltc_nr); > nvkm_wr32(device, 0x17e000, ltc->ltc_nr); > nvkm_wr32(device, 0x100800, ltc->ltc_nr); > > - spec = dev_iommu_fwspec_get(device->dev); > - if (spec) { > - u32 sid = spec->ids[0] & 0xffff; > - > + if (tegra_dev_iommu_get_stream_id(device->dev, &sid)) { > /* stream ID */ > nvkm_wr32(device, 0x160000, sid << 2); We could probably also remove the comment now since the function and variable names make it obvious what's being written here. > } > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c > index 533f85a4b2bdb7..3e4fbe94dd666e 100644 > --- a/drivers/memory/tegra/tegra186.c > +++ b/drivers/memory/tegra/tegra186.c > @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(struct tegra_mc *mc, > static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) > { > #if IS_ENABLED(CONFIG_IOMMU_API) > - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > struct of_phandle_args args; > unsigned int i, index = 0; > + u32 sid; > > + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid)); I know the code previously didn't check for any errors, but we may want to do so now. If tegra_dev_iommu_get_stream_id() ever fails we may end up writing some undefined value into the override register. I'm also unsure if WARN_ON() is appropriate here. I vaguely recall that ->probe_device() was called for all devices on the bus and not all of them may have been associated with the IOMMU. Not all of them may in fact access memory in the first place. Perhaps I'm misremembering and the IOMMU core now takes care of only calling this when fwspec is indeed valid? Thierry
On Wed, Nov 29, 2023 at 05:23:13PM +0100, Thierry Reding wrote: > > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c > > index 533f85a4b2bdb7..3e4fbe94dd666e 100644 > > --- a/drivers/memory/tegra/tegra186.c > > +++ b/drivers/memory/tegra/tegra186.c > > @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(struct tegra_mc *mc, > > static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) > > { > > #if IS_ENABLED(CONFIG_IOMMU_API) > > - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > > struct of_phandle_args args; > > unsigned int i, index = 0; > > + u32 sid; > > > > + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid)); > > I know the code previously didn't check for any errors, but we may want > to do so now. If tegra_dev_iommu_get_stream_id() ever fails we may end > up writing some undefined value into the override register. My assumption was it never fails otherwise this probably already doesn't work? > I'm also unsure if WARN_ON() is appropriate here. I vaguely recall that > ->probe_device() was called for all devices on the bus and not all of > them may have been associated with the IOMMU. Not all of them may in > fact access memory in the first place. So you are thinkin that of_parse_phandle_with_args() is a NOP sometimes so it will tolerate the failure? Seems like the best thing to do is just continue to ignore it then? > Perhaps I'm misremembering and the IOMMU core now takes care of only > calling this when fwspec is indeed valid? Can't advise, I have no idea what tegra_mc_ops is for :) Jason
On Wed, Nov 29, 2023 at 03:26:03PM -0400, Jason Gunthorpe wrote: > On Wed, Nov 29, 2023 at 05:23:13PM +0100, Thierry Reding wrote: > > > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c > > > index 533f85a4b2bdb7..3e4fbe94dd666e 100644 > > > --- a/drivers/memory/tegra/tegra186.c > > > +++ b/drivers/memory/tegra/tegra186.c > > > @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(struct tegra_mc *mc, > > > static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) > > > { > > > #if IS_ENABLED(CONFIG_IOMMU_API) > > > - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > > > struct of_phandle_args args; > > > unsigned int i, index = 0; > > > + u32 sid; > > > > > > + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid)); > > > > I know the code previously didn't check for any errors, but we may want > > to do so now. If tegra_dev_iommu_get_stream_id() ever fails we may end > > up writing some undefined value into the override register. > > My assumption was it never fails otherwise this probably already > doesn't work? I guess the point I was trying to make is that previously we would not have written anything to the stream ID register and so ignoring the error here might end up writing to a register that previously we would not have written to. Looking at the current code more closely I see now that the reason why we wouldn't have written to the register is because we would've crashed before. So I think this okay. > > > I'm also unsure if WARN_ON() is appropriate here. I vaguely recall that > > ->probe_device() was called for all devices on the bus and not all of > > them may have been associated with the IOMMU. Not all of them may in > > fact access memory in the first place. > > So you are thinkin that of_parse_phandle_with_args() is a NOP > sometimes so it will tolerate the failure? > > Seems like the best thing to do is just continue to ignore it then? Yeah, exactly. It would've just skipped over everything, basically. > > Perhaps I'm misremembering and the IOMMU core now takes care of only > > calling this when fwspec is indeed valid? > > Can't advise, I have no idea what tegra_mc_ops is for :) In a nutshell, it's a hook that allows us to configure the memory controller when a device is attached to the IOMMU. The memory controller contains a set of registers that specify which memory client uses which stream ID by default. For some devices this can be overridden (which is where tegra_dev_iommu_get_stream_id() comes into play in those drivers) and for other devices we can't override, which is when the memory controller defaults come into play. Anyway, I took a closer look at this and ran some tests. Turns out that tegra186_mc_probe_device() really only gets called for devices that have their fwspec properly initialized anyway, so I don't think there's anything special we need to do here. Strictly from a static analysis point of view I suppose we could now have a situation that sid is uninitialized when the call to tegra_dev_iommu_get_stream_id() fails and so using it in the loop is not correct, theoretically, but I think that's just not a case that we'll ever hit in practice. So either way is fine with me. I have a slight preference for just returning 0 in case tegra_dev_iommu_get_stream_id() fails, because it's simple to do and avoids any of these (theoretical) ambiguities. So whichever way you decide: Reviewed-by: Thierry Reding <treding@nvidia.com>
diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c index fa4d4142a68a21..88547a23825b18 100644 --- a/drivers/dma/tegra186-gpc-dma.c +++ b/drivers/dma/tegra186-gpc-dma.c @@ -1348,8 +1348,8 @@ static int tegra_dma_program_sid(struct tegra_dma_channel *tdc, int stream_id) static int tegra_dma_probe(struct platform_device *pdev) { const struct tegra_dma_chip_data *cdata = NULL; - struct iommu_fwspec *iommu_spec; - unsigned int stream_id, i; + unsigned int i; + u32 stream_id; struct tegra_dma *tdma; int ret; @@ -1378,12 +1378,10 @@ static int tegra_dma_probe(struct platform_device *pdev) tdma->dma_dev.dev = &pdev->dev; - iommu_spec = dev_iommu_fwspec_get(&pdev->dev); - if (!iommu_spec) { + if (!tegra_dev_iommu_get_stream_id(&pdev->dev, &stream_id)) { dev_err(&pdev->dev, "Missing iommu stream-id\n"); return -EINVAL; } - stream_id = iommu_spec->ids[0] & 0xffff; ret = device_property_read_u32(&pdev->dev, "dma-channel-mask", &tdma->chan_mask); diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c index e7e8fdf3adab7a..b40fd1dbb21617 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c @@ -28,16 +28,13 @@ static void gp10b_ltc_init(struct nvkm_ltc *ltc) { struct nvkm_device *device = ltc->subdev.device; - struct iommu_fwspec *spec; + u32 sid; nvkm_wr32(device, 0x17e27c, ltc->ltc_nr); nvkm_wr32(device, 0x17e000, ltc->ltc_nr); nvkm_wr32(device, 0x100800, ltc->ltc_nr); - spec = dev_iommu_fwspec_get(device->dev); - if (spec) { - u32 sid = spec->ids[0] & 0xffff; - + if (tegra_dev_iommu_get_stream_id(device->dev, &sid)) { /* stream ID */ nvkm_wr32(device, 0x160000, sid << 2); } diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c index 533f85a4b2bdb7..3e4fbe94dd666e 100644 --- a/drivers/memory/tegra/tegra186.c +++ b/drivers/memory/tegra/tegra186.c @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(struct tegra_mc *mc, static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) { #if IS_ENABLED(CONFIG_IOMMU_API) - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct of_phandle_args args; unsigned int i, index = 0; + u32 sid; + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid)); while (!of_parse_phandle_with_args(dev->of_node, "interconnects", "#interconnect-cells", index, &args)) { if (args.np == mc->dev->of_node && args.args_count != 0) { for (i = 0; i < mc->soc->num_clients; i++) { const struct tegra_mc_client *client = &mc->soc->clients[i]; - if (client->id == args.args[0]) { - u32 sid = fwspec->ids[0] & MC_SID_STREAMID_OVERRIDE_MASK; - - tegra186_mc_client_sid_override(mc, client, sid); - } + if (client->id == args.args[0]) + tegra186_mc_client_sid_override( + mc, client, + sid & MC_SID_STREAMID_OVERRIDE_MASK); } }
This API was defined to formalize the access to internal iommu details on some Tegra SOCs, but a few callers got missed. Add them. The helper already masks by 0xFFFF so remove this code from the callers. Suggested-by: Thierry Reding <thierry.reding@gmail.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/dma/tegra186-gpc-dma.c | 8 +++----- drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 7 ++----- drivers/memory/tegra/tegra186.c | 12 ++++++------ 3 files changed, 11 insertions(+), 16 deletions(-)