On Sat, 16 Nov 2024 20:22:53 +0100 Andrew Lunn <andrew@xxxxxxx> wrote: > On Fri, Nov 15, 2024 at 01:59:40PM -0500, Parker Newman wrote: > > On Fri, 15 Nov 2024 18:17:07 +0100 > > Paolo Abeni <pabeni@xxxxxxxxxx> wrote: > > > > > On 11/15/24 17:31, Parker Newman wrote: > > > > From: Parker Newman <pnewman@xxxxxxxxxxxxxxx> > > > > > > > > Read the iommu stream id from device tree rather than hard coding to mgbe0. > > > > Fixes kernel panics when using mgbe controllers other than mgbe0. > > > > > > It's better to include the full Oops backtrace, possibly decoded. > > > > > > > Will do, there are many different ones but I can add the most common. > > > > > > Tested with Orin AGX 64GB module on Connect Tech Forge carrier board. > > > > > > Since this looks like a fix, you should include a suitable 'Fixes' tag > > > here, and specify the 'net' target tree in the subj prefix. > > > > > > > Sorry I missed the "net" tag. > > > > The bug has existed since dwmac-tegra.c was added. I can add a Fixes tag but > > in the past I was told they aren't needed in that situation? > > > > > > @@ -241,6 +243,12 @@ static int tegra_mgbe_probe(struct platform_device *pdev) > > > > if (IS_ERR(mgbe->xpcs)) > > > > return PTR_ERR(mgbe->xpcs); > > > > > > > > + /* get controller's stream id from iommu property in device tree */ > > > > + if (!tegra_dev_iommu_get_stream_id(mgbe->dev, &mgbe->iommu_sid)) { > > > > + dev_err(mgbe->dev, "failed to get iommu stream id\n"); > > > > + return -EINVAL; > > > > + } > > > > > > I *think* it would be better to fallback (possibly with a warning or > > > notice) to the previous default value when the device tree property is > > > not available, to avoid regressions. > > > > > > > I debated this as well... In theory the iommu must be setup for the > > mgbe controller to work anyways. Doing it this way means the worst case is > > probe() fails and you lose an ethernet port. > > New DT properties are always optional. Take the example of a board > only using a single controller. It should happily work. It probably > does not have this property because it is not needed. Your change is > likely to cause a regression on such a board. > > Also, is a binding patch needed? > > Andrew This is not a new dt property, the "iommus" property is an existing property that is parsed by the Nvidia implementation of the arm-smmu driver. Here is a snippet from the device tree: smmu_niso0: iommu@12000000 { compatible = "nvidia,tegra234-smmu", "nvidia,smmu-500"; ... } /* MGBE0 */ ethernet@6800000 { compatible = "nvidia,tegra234-mgbe"; ... iommus = <&smmu_niso0 TEGRA234_SID_MGBE>; ... } /* MGBE1 */ ethernet@6900000 { compatible = "nvidia,tegra234-mgbe"; ... iommus = <&smmu_niso0 TEGRA234_SID_MGBE_VF1>; ... } The 2nd field of the iommus propert is the "Stream ID" which arm-smmu stores in the device's struct iommu_fwspec *fwspec. This is what the existing tegra_dev_iommu_get_stream_id() function uses to get the SID. If the iommus property is missing completely from the MGBE's device tree node it causes secure read/write errors which spam the kernel log and can cause crashes. I can add the fallback in V2 with a warning if that is preferred. Thanks, Parker