| Message ID | 6863998.WsTHer29ZI@avalon (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Laurent, On 5/18/2017 4:09 PM, Laurent Pinchart wrote: > Hi Sricharan, > > Thank you for the patch. > > On Thursday 18 May 2017 15:37:09 Sricharan R wrote: >> While deferring the probe of IOMMU masters, >> xlate and add_device callback can pass back error values >> like -ENODEV, which means IOMMU cannot be connected >> with that master for real reasons. So rather than >> killing the master's probe for such errors, just >> ignore the errors and let the master work without >> an IOMMU. > > I don't think this is a good idea. Why should we allow IOMMU drivers to return > an error if we're always going to ignore the error value ? That will lead to > drivers implementing slightly different behaviours, which will be painful the > day we'll need to start acting based on the error value. > The of_iommu_configure interface, before this series, was returning either correct 'iommu_ops' or NULL. Also there was no return value from of_dma_configure which calls of_iommu_configure. This means that if we block only -ENODEV now and let the other errors, the probe of the master devices can be killed for reasons apart from deferring. This would be a change in behavior introduced. All of xlate, add_device, of_pci_map_rid and others can return values apart from -ENODEV. So was thinking that restoring the old behavior, except for returning EPROBE_DEFER was the better thing to do ? Regards, Sricharan > At the very least, if you want to give a specific meaning to -ENODEV, you > should check for that value specifically and not ignore all errors other than > -EPROBE_DEFER. You also need to document the meaning of the error value. This > can be done in the documentation of the of_xlate operation in > include/linux/iommu.h: > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 2cb54adc4a33..6ba553e7384a 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -181,7 +181,6 @@ struct iommu_resv_region { > * @domain_window_disable: Disable a particular window for a domain > * @domain_set_windows: Set the number of windows for a domain > * @domain_get_windows: Return the number of windows for a domain > - * @of_xlate: add OF master IDs to iommu grouping > * @pgsize_bitmap: bitmap of all possible supported page sizes > */ > struct iommu_ops { > @@ -224,6 +223,11 @@ struct iommu_ops { > /* Get the number of windows per domain */ > u32 (*domain_get_windows)(struct iommu_domain *domain); > > + /** > + * @of_xlate: > + * > + * Add OF master IDs to iommu grouping. > + */ > int (*of_xlate)(struct device *dev, struct of_phandle_args *args); > > unsigned long pgsize_bitmap; > > > And add documentation for the error codes there. > > If you want to ignore some errors returned from the add_device operation you > should document it similarly, and in particular document which error check(s) > need to be performed by of_xlate and which are the responsibility of > add_device. > >> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred >> probing or error") >> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> >> Tested-by: Magnus Damn <magnus.damn@gmail.com> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> --- >> [V2] Corrected spelling/case in commit log >> >> drivers/iommu/of_iommu.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >> index e6e9bec..f0d22c0 100644 >> --- a/drivers/iommu/of_iommu.c >> +++ b/drivers/iommu/of_iommu.c >> @@ -237,6 +237,12 @@ const struct iommu_ops *of_iommu_configure(struct >> device *dev, ops = ERR_PTR(err); >> } >> >> + /* Ignore all other errors apart from EPROBE_DEFER */ >> + if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) { >> + dev_info(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops)); >> + ops = NULL; >> + } >> + >> return ops; >> } >
Hi Sricharan, On Thursday 18 May 2017 17:26:14 Sricharan R wrote: > On 5/18/2017 4:09 PM, Laurent Pinchart wrote: > > On Thursday 18 May 2017 15:37:09 Sricharan R wrote: > >> While deferring the probe of IOMMU masters, > >> xlate and add_device callback can pass back error values > >> like -ENODEV, which means IOMMU cannot be connected > >> with that master for real reasons. So rather than > >> killing the master's probe for such errors, just > >> ignore the errors and let the master work without > >> an IOMMU. > > > > I don't think this is a good idea. Why should we allow IOMMU drivers to > > return an error if we're always going to ignore the error value ? That > > will lead to drivers implementing slightly different behaviours, which > > will be painful the day we'll need to start acting based on the error > > value. > > The of_iommu_configure interface, before this series, was returning either > correct 'iommu_ops' or NULL. Also there was no return value from > of_dma_configure which calls of_iommu_configure. This means that if we block > only -ENODEV now and let the other errors, the probe of the master devices > can be killed for reasons apart from deferring. This would be a change in > behavior introduced. All of xlate, add_device, of_pci_map_rid and others > can return values apart from -ENODEV. So was thinking that restoring the > old behavior, except for returning EPROBE_DEFER was the better thing to do > ? We went from a situation where of_iommu_configure() could return either valid operations in the case the device was to be handled by the IOMMU or NULL otherwise, to a situation where we needed a third option for probe deferral. The way we've done this, through error pointers, allows lots of other errors to be returned as well from the of_xlate and add_device operations. There is currently no use for returning error codes other than -EPROBE_DEFFER from of_iommu_configure(), so your proposal is to block errors returned from the of_xlate and add_device operations inside of_iommu_configure(). My point is that, by doing so, we allow IOMMU drivers to return random error codes that are then ignored. I believe this can cause problems in the future when we will need to extend the API and standardize more error codes, as by then IOMMU drivers will return random errors (they actually do so already to some extent). For of_xlate I agree with you to some extent. v4.11 just checked whether of_xlate succeeded or not, and just didn't use the IOMMU in case it failed. The exact error value was ignored, and drivers already return random errors. Going back to the v4.11 behaviour is what we need to do in the short-term, even if I believe we should standardize the error values returned from of_xlate after v4.12. For add_device, however, the situation is a bit different. The add_device operation is called from the IOMMU bus notifier, and the -ENODEV error is ignored by add_iommu_group(). Any other error will cause bus_set_iommu() to fail, which makes IOMMU probing fail for the drivers that check the return value of bus_set_iommu() (some of them don't :-/). Fixing all this properly requires standardizing the error codes, and going through the existing IOMMU drivers to comply with the standardized behaviour. While this shouldn't be very difficult, it's likely not material for a v4.12- rc fix. We will thus likely need to merge this patch (or something very similar to it), but I'd really like to see this fixed properly for v4.13. > > At the very least, if you want to give a specific meaning to -ENODEV, you > > should check for that value specifically and not ignore all errors other > > than -EPROBE_DEFER. You also need to document the meaning of the error > > value. This can be done in the documentation of the of_xlate operation in > > include/linux/iommu.h: > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > index 2cb54adc4a33..6ba553e7384a 100644 > > --- a/include/linux/iommu.h > > +++ b/include/linux/iommu.h > > @@ -181,7 +181,6 @@ struct iommu_resv_region { > > * @domain_window_disable: Disable a particular window for a domain > > * @domain_set_windows: Set the number of windows for a domain > > * @domain_get_windows: Return the number of windows for a domain > > - * @of_xlate: add OF master IDs to iommu grouping > > * @pgsize_bitmap: bitmap of all possible supported page sizes > > */ > > > > struct iommu_ops { > > @@ -224,6 +223,11 @@ struct iommu_ops { > > /* Get the number of windows per domain */ > > u32 (*domain_get_windows)(struct iommu_domain *domain); > > > > + /** > > + * @of_xlate: > > + * > > + * Add OF master IDs to iommu grouping. > > + */ > > int (*of_xlate)(struct device *dev, struct of_phandle_args *args); > > > > unsigned long pgsize_bitmap; > > > > And add documentation for the error codes there. > > > > If you want to ignore some errors returned from the add_device operation > > you should document it similarly, and in particular document which error > > check(s) need to be performed by of_xlate and which are the > > responsibility of add_device. > > > >> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with > >> deferred probing or error") > >> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > >> Tested-by: Magnus Damn <magnus.damn@gmail.com> > >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> > >> --- > >> [V2] Corrected spelling/case in commit log > >> > >> drivers/iommu/of_iommu.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > >> index e6e9bec..f0d22c0 100644 > >> --- a/drivers/iommu/of_iommu.c > >> +++ b/drivers/iommu/of_iommu.c > >> @@ -237,6 +237,12 @@ const struct iommu_ops *of_iommu_configure(struct > >> device *dev, ops = ERR_PTR(err); > >> } > >> > >> + /* Ignore all other errors apart from EPROBE_DEFER */ > >> + if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) { > >> + dev_info(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops)); > >> + ops = NULL; > >> + } > >> + > >> return ops; > >> }
Hi Laurent, On 5/18/2017 6:00 PM, Laurent Pinchart wrote: > Hi Sricharan, > > On Thursday 18 May 2017 17:26:14 Sricharan R wrote: >> On 5/18/2017 4:09 PM, Laurent Pinchart wrote: >>> On Thursday 18 May 2017 15:37:09 Sricharan R wrote: >>>> While deferring the probe of IOMMU masters, >>>> xlate and add_device callback can pass back error values >>>> like -ENODEV, which means IOMMU cannot be connected >>>> with that master for real reasons. So rather than >>>> killing the master's probe for such errors, just >>>> ignore the errors and let the master work without >>>> an IOMMU. >>> >>> I don't think this is a good idea. Why should we allow IOMMU drivers to >>> return an error if we're always going to ignore the error value ? That >>> will lead to drivers implementing slightly different behaviours, which >>> will be painful the day we'll need to start acting based on the error >>> value. >> >> The of_iommu_configure interface, before this series, was returning either >> correct 'iommu_ops' or NULL. Also there was no return value from >> of_dma_configure which calls of_iommu_configure. This means that if we block >> only -ENODEV now and let the other errors, the probe of the master devices >> can be killed for reasons apart from deferring. This would be a change in >> behavior introduced. All of xlate, add_device, of_pci_map_rid and others >> can return values apart from -ENODEV. So was thinking that restoring the >> old behavior, except for returning EPROBE_DEFER was the better thing to do >> ? > > We went from a situation where of_iommu_configure() could return either valid > operations in the case the device was to be handled by the IOMMU or NULL > otherwise, to a situation where we needed a third option for probe deferral. > The way we've done this, through error pointers, allows lots of other errors > to be returned as well from the of_xlate and add_device operations. > right, this was difference in the behavior now. > There is currently no use for returning error codes other than -EPROBE_DEFFER > from of_iommu_configure(), so your proposal is to block errors returned from > the of_xlate and add_device operations inside of_iommu_configure(). My point > is that, by doing so, we allow IOMMU drivers to return random error codes that > are then ignored. I believe this can cause problems in the future when we will > need to extend the API and standardize more error codes, as by then IOMMU > drivers will return random errors (they actually do so already to some > extent). > > For of_xlate I agree with you to some extent. v4.11 just checked whether > of_xlate succeeded or not, and just didn't use the IOMMU in case it failed. > The exact error value was ignored, and drivers already return random errors. > Going back to the v4.11 behaviour is what we need to do in the short-term, > even if I believe we should standardize the error values returned from > of_xlate after v4.12. > > For add_device, however, the situation is a bit different. The add_device > operation is called from the IOMMU bus notifier, and the -ENODEV error is > ignored by add_iommu_group(). Any other error will cause bus_set_iommu() to > fail, which makes IOMMU probing fail for the drivers that check the return > value of bus_set_iommu() (some of them don't :-/). > > Fixing all this properly requires standardizing the error codes, and going > through the existing IOMMU drivers to comply with the standardized behaviour. I understand your concern on standardizing the error codes from xlate, add_device, others and handling them properly. As you said there are quite some errors returned from them today. Also another thing is standardizing the behavior of of_iommu_configure itself. So that API serves to connect a device to its correct iommu_ops. When that's not possible, what should be the output and how should that be handled by the caller. The current behavior is to either 1) connect to correct ops or 2) wait for it or 3) progress further with plain/default dma_ops. Anyways as you said standardizing the iommu api ops, would make the of_iommu_configure handling more specific. Having said that i think similar fix needs to be done for acpi's iort_iommu_configure as well. > While this shouldn't be very difficult, it's likely not material for a v4.12- > rc fix. We will thus likely need to merge this patch (or something very > similar to it), but I'd really like to see this fixed properly for v4.13. > When you say "merge this patch (or something similar)", is that about documenting the error values for of_xlate and add_device that you showed down below (or) about the patch in discussion ? Regards, Sricharan >>> At the very least, if you want to give a specific meaning to -ENODEV, you >>> should check for that value specifically and not ignore all errors other >>> than -EPROBE_DEFER. You also need to document the meaning of the error >>> value. This can be done in the documentation of the of_xlate operation in >>> include/linux/iommu.h: >>> >>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >>> index 2cb54adc4a33..6ba553e7384a 100644 >>> --- a/include/linux/iommu.h >>> +++ b/include/linux/iommu.h >>> @@ -181,7 +181,6 @@ struct iommu_resv_region { >>> * @domain_window_disable: Disable a particular window for a domain >>> * @domain_set_windows: Set the number of windows for a domain >>> * @domain_get_windows: Return the number of windows for a domain >>> - * @of_xlate: add OF master IDs to iommu grouping >>> * @pgsize_bitmap: bitmap of all possible supported page sizes >>> */ >>> >>> struct iommu_ops { >>> @@ -224,6 +223,11 @@ struct iommu_ops { >>> /* Get the number of windows per domain */ >>> u32 (*domain_get_windows)(struct iommu_domain *domain); >>> >>> + /** >>> + * @of_xlate: >>> + * >>> + * Add OF master IDs to iommu grouping. >>> + */ >>> int (*of_xlate)(struct device *dev, struct of_phandle_args *args); >>> >>> unsigned long pgsize_bitmap; >>> >>> And add documentation for the error codes there. >>> >>> If you want to ignore some errors returned from the add_device operation >>> you should document it similarly, and in particular document which error >>> check(s) need to be performed by of_xlate and which are the >>> responsibility of add_device. >>> >>>> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with >>>> deferred probing or error") >>>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> >>>> Tested-by: Magnus Damn <magnus.damn@gmail.com> >>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>> --- >>>> [V2] Corrected spelling/case in commit log >>>> >>>> drivers/iommu/of_iommu.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >>>> index e6e9bec..f0d22c0 100644 >>>> --- a/drivers/iommu/of_iommu.c >>>> +++ b/drivers/iommu/of_iommu.c >>>> @@ -237,6 +237,12 @@ const struct iommu_ops *of_iommu_configure(struct >>>> device *dev, ops = ERR_PTR(err); >>>> } >>>> >>>> + /* Ignore all other errors apart from EPROBE_DEFER */ >>>> + if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) { >>>> + dev_info(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops)); >>>> + ops = NULL; >>>> + } >>>> + >>>> return ops; >>>> } >
Hi Sricharan On Thursday 18 May 2017 19:08:12 Sricharan R wrote: > On 5/18/2017 6:00 PM, Laurent Pinchart wrote: > > On Thursday 18 May 2017 17:26:14 Sricharan R wrote: > >> On 5/18/2017 4:09 PM, Laurent Pinchart wrote: > >>> On Thursday 18 May 2017 15:37:09 Sricharan R wrote: > >>>> While deferring the probe of IOMMU masters, > >>>> xlate and add_device callback can pass back error values > >>>> like -ENODEV, which means IOMMU cannot be connected > >>>> with that master for real reasons. So rather than > >>>> killing the master's probe for such errors, just > >>>> ignore the errors and let the master work without > >>>> an IOMMU. > >>> > >>> I don't think this is a good idea. Why should we allow IOMMU drivers to > >>> return an error if we're always going to ignore the error value ? That > >>> will lead to drivers implementing slightly different behaviours, which > >>> will be painful the day we'll need to start acting based on the error > >>> value. > >> > >> The of_iommu_configure interface, before this series, was returning > >> either correct 'iommu_ops' or NULL. Also there was no return value from > >> of_dma_configure which calls of_iommu_configure. This means that if we > >> block only -ENODEV now and let the other errors, the probe of the master > >> devices can be killed for reasons apart from deferring. This would be a > >> change in behavior introduced. All of xlate, add_device, of_pci_map_rid > >> and others can return values apart from -ENODEV. So was thinking that > >> restoring the old behavior, except for returning EPROBE_DEFER was the > >> better thing to do ? > > > > We went from a situation where of_iommu_configure() could return either > > valid operations in the case the device was to be handled by the IOMMU or > > NULL otherwise, to a situation where we needed a third option for probe > > deferral. The way we've done this, through error pointers, allows lots of > > other errors to be returned as well from the of_xlate and add_device > > operations. > > right, this was difference in the behavior now. > > > There is currently no use for returning error codes other than > > -EPROBE_DEFFER from of_iommu_configure(), so your proposal is to block > > errors returned from the of_xlate and add_device operations inside > > of_iommu_configure(). My point is that, by doing so, we allow IOMMU > > drivers to return random error codes that are then ignored. I believe > > this can cause problems in the future when we will need to extend the API > > and standardize more error codes, as by then IOMMU drivers will return > > random errors (they actually do so already to some extent). > > > > For of_xlate I agree with you to some extent. v4.11 just checked whether > > of_xlate succeeded or not, and just didn't use the IOMMU in case it > > failed. The exact error value was ignored, and drivers already return > > random errors. Going back to the v4.11 behaviour is what we need to do in > > the short-term, even if I believe we should standardize the error values > > returned from of_xlate after v4.12. > > > > For add_device, however, the situation is a bit different. The add_device > > operation is called from the IOMMU bus notifier, and the -ENODEV error is > > ignored by add_iommu_group(). Any other error will cause bus_set_iommu() > > to fail, which makes IOMMU probing fail for the drivers that check the > > return value of bus_set_iommu() (some of them don't :-/). > > > > Fixing all this properly requires standardizing the error codes, and going > > through the existing IOMMU drivers to comply with the standardized > > behaviour. > > I understand your concern on standardizing the error codes from xlate, > add_device, others and handling them properly. As you said there are quite > some errors returned from them today. Also another thing is standardizing > the behavior of of_iommu_configure itself. So that API serves to connect a > device to its correct iommu_ops. When that's not possible, what should be > the output and how should that be handled by the caller. The current > behavior is to either 1) connect to correct ops or 2) wait for it or 3) > progress further with plain/default dma_ops. Anyways as you said > standardizing the iommu api ops, would make the of_iommu_configure handling > more specific. Having said that i think similar fix needs to be done for > acpi's iort_iommu_configure as well. I'm less knowledgeable about ACPI but I think you're right. Would you like to tackle this for v4.13 ? :-) > > While this shouldn't be very difficult, it's likely not material for a > > v4.12- rc fix. We will thus likely need to merge this patch (or something > > very similar to it), but I'd really like to see this fixed properly for > > v4.13. > > When you say "merge this patch (or something similar)", is that about > documenting the error values for of_xlate and add_device that you showed > down below (or) about the patch in discussion ? I meant the patch we're discussing, "[PATCH V2 2/3] iommu: of: Ignore all errors except EPROBE_DEFER". Sorry for the confusion. One more comment about this below. > >>> At the very least, if you want to give a specific meaning to -ENODEV, > >>> you should check for that value specifically and not ignore all errors > >>> other than -EPROBE_DEFER. You also need to document the meaning of the > >>> error value. This can be done in the documentation of the of_xlate > >>> operation in include/linux/iommu.h: > >>> > >>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h > >>> index 2cb54adc4a33..6ba553e7384a 100644 > >>> --- a/include/linux/iommu.h > >>> +++ b/include/linux/iommu.h > >>> @@ -181,7 +181,6 @@ struct iommu_resv_region { > >>> * @domain_window_disable: Disable a particular window for a domain > >>> * @domain_set_windows: Set the number of windows for a domain > >>> * @domain_get_windows: Return the number of windows for a domain > >>> - * @of_xlate: add OF master IDs to iommu grouping > >>> * @pgsize_bitmap: bitmap of all possible supported page sizes > >>> */ > >>> struct iommu_ops { > >>> @@ -224,6 +223,11 @@ struct iommu_ops { > >>> /* Get the number of windows per domain */ > >>> u32 (*domain_get_windows)(struct iommu_domain *domain); > >>> > >>> + /** > >>> + * @of_xlate: > >>> + * > >>> + * Add OF master IDs to iommu grouping. > >>> + */ > >>> int (*of_xlate)(struct device *dev, struct of_phandle_args *args); > >>> > >>> unsigned long pgsize_bitmap; > >>> > >>> And add documentation for the error codes there. > >>> > >>> If you want to ignore some errors returned from the add_device operation > >>> you should document it similarly, and in particular document which error > >>> check(s) need to be performed by of_xlate and which are the > >>> responsibility of add_device. > >>> > >>>> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with > >>>> deferred probing or error") > >>>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > >>>> Tested-by: Magnus Damn <magnus.damn@gmail.com> > >>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> > >>>> --- > >>>> [V2] Corrected spelling/case in commit log > >>>> > >>>> drivers/iommu/of_iommu.c | 6 ++++++ > >>>> 1 file changed, 6 insertions(+) > >>>> > >>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > >>>> index e6e9bec..f0d22c0 100644 > >>>> --- a/drivers/iommu/of_iommu.c > >>>> +++ b/drivers/iommu/of_iommu.c > >>>> @@ -237,6 +237,12 @@ const struct iommu_ops *of_iommu_configure(struct > >>>> device *dev, > >>>> ops = ERR_PTR(err); > >>>> } > >>>> > >>>> + /* Ignore all other errors apart from EPROBE_DEFER */ > >>>> + if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) { > >>>> + dev_info(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops)); I would turn this into a dev_dbg(), as at least the -ENODEV error returned from add_device has a defined meaning (see the comment in add_iommu_group()) and is in my opinion not a condition that should be reported in the kernel log by default. > >>>> + ops = NULL; > >>>> + } > >>>> + > >>>> return ops; > >>>> }
Hi Laurent, On 5/18/2017 7:13 PM, Laurent Pinchart wrote: > Hi Sricharan > > On Thursday 18 May 2017 19:08:12 Sricharan R wrote: >> On 5/18/2017 6:00 PM, Laurent Pinchart wrote: >>> On Thursday 18 May 2017 17:26:14 Sricharan R wrote: >>>> On 5/18/2017 4:09 PM, Laurent Pinchart wrote: >>>>> On Thursday 18 May 2017 15:37:09 Sricharan R wrote: >>>>>> While deferring the probe of IOMMU masters, >>>>>> xlate and add_device callback can pass back error values >>>>>> like -ENODEV, which means IOMMU cannot be connected >>>>>> with that master for real reasons. So rather than >>>>>> killing the master's probe for such errors, just >>>>>> ignore the errors and let the master work without >>>>>> an IOMMU. >>>>> >>>>> I don't think this is a good idea. Why should we allow IOMMU drivers to >>>>> return an error if we're always going to ignore the error value ? That >>>>> will lead to drivers implementing slightly different behaviours, which >>>>> will be painful the day we'll need to start acting based on the error >>>>> value. >>>> >>>> The of_iommu_configure interface, before this series, was returning >>>> either correct 'iommu_ops' or NULL. Also there was no return value from >>>> of_dma_configure which calls of_iommu_configure. This means that if we >>>> block only -ENODEV now and let the other errors, the probe of the master >>>> devices can be killed for reasons apart from deferring. This would be a >>>> change in behavior introduced. All of xlate, add_device, of_pci_map_rid >>>> and others can return values apart from -ENODEV. So was thinking that >>>> restoring the old behavior, except for returning EPROBE_DEFER was the >>>> better thing to do ? >>> >>> We went from a situation where of_iommu_configure() could return either >>> valid operations in the case the device was to be handled by the IOMMU or >>> NULL otherwise, to a situation where we needed a third option for probe >>> deferral. The way we've done this, through error pointers, allows lots of >>> other errors to be returned as well from the of_xlate and add_device >>> operations. >> >> right, this was difference in the behavior now. >> >>> There is currently no use for returning error codes other than >>> -EPROBE_DEFFER from of_iommu_configure(), so your proposal is to block >>> errors returned from the of_xlate and add_device operations inside >>> of_iommu_configure(). My point is that, by doing so, we allow IOMMU >>> drivers to return random error codes that are then ignored. I believe >>> this can cause problems in the future when we will need to extend the API >>> and standardize more error codes, as by then IOMMU drivers will return >>> random errors (they actually do so already to some extent). >>> >>> For of_xlate I agree with you to some extent. v4.11 just checked whether >>> of_xlate succeeded or not, and just didn't use the IOMMU in case it >>> failed. The exact error value was ignored, and drivers already return >>> random errors. Going back to the v4.11 behaviour is what we need to do in >>> the short-term, even if I believe we should standardize the error values >>> returned from of_xlate after v4.12. >>> >>> For add_device, however, the situation is a bit different. The add_device >>> operation is called from the IOMMU bus notifier, and the -ENODEV error is >>> ignored by add_iommu_group(). Any other error will cause bus_set_iommu() >>> to fail, which makes IOMMU probing fail for the drivers that check the >>> return value of bus_set_iommu() (some of them don't :-/). >>> >>> Fixing all this properly requires standardizing the error codes, and going >>> through the existing IOMMU drivers to comply with the standardized >>> behaviour. >> >> I understand your concern on standardizing the error codes from xlate, >> add_device, others and handling them properly. As you said there are quite >> some errors returned from them today. Also another thing is standardizing >> the behavior of of_iommu_configure itself. So that API serves to connect a >> device to its correct iommu_ops. When that's not possible, what should be >> the output and how should that be handled by the caller. The current >> behavior is to either 1) connect to correct ops or 2) wait for it or 3) >> progress further with plain/default dma_ops. Anyways as you said >> standardizing the iommu api ops, would make the of_iommu_configure handling >> more specific. Having said that i think similar fix needs to be done for >> acpi's iort_iommu_configure as well. > > I'm less knowledgeable about ACPI but I think you're right. Would you like to > tackle this for v4.13 ? :-) Will add the fix for ACPI now in this series. ok, Will really see if i can address the standardizing part for 4.13. > >>> While this shouldn't be very difficult, it's likely not material for a >>> v4.12- rc fix. We will thus likely need to merge this patch (or something >>> very similar to it), but I'd really like to see this fixed properly for >>> v4.13. >> >> When you say "merge this patch (or something similar)", is that about >> documenting the error values for of_xlate and add_device that you showed >> down below (or) about the patch in discussion ? > > I meant the patch we're discussing, "[PATCH V2 2/3] iommu: of: Ignore all > errors except EPROBE_DEFER". Sorry for the confusion. One more comment about > this below. ok. > >>>>> At the very least, if you want to give a specific meaning to -ENODEV, >>>>> you should check for that value specifically and not ignore all errors >>>>> other than -EPROBE_DEFER. You also need to document the meaning of the >>>>> error value. This can be done in the documentation of the of_xlate >>>>> operation in include/linux/iommu.h: >>>>> >>>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >>>>> index 2cb54adc4a33..6ba553e7384a 100644 >>>>> --- a/include/linux/iommu.h >>>>> +++ b/include/linux/iommu.h >>>>> @@ -181,7 +181,6 @@ struct iommu_resv_region { >>>>> * @domain_window_disable: Disable a particular window for a domain >>>>> * @domain_set_windows: Set the number of windows for a domain >>>>> * @domain_get_windows: Return the number of windows for a domain >>>>> - * @of_xlate: add OF master IDs to iommu grouping >>>>> * @pgsize_bitmap: bitmap of all possible supported page sizes >>>>> */ >>>>> struct iommu_ops { >>>>> @@ -224,6 +223,11 @@ struct iommu_ops { >>>>> /* Get the number of windows per domain */ >>>>> u32 (*domain_get_windows)(struct iommu_domain *domain); >>>>> >>>>> + /** >>>>> + * @of_xlate: >>>>> + * >>>>> + * Add OF master IDs to iommu grouping. >>>>> + */ >>>>> int (*of_xlate)(struct device *dev, struct of_phandle_args *args); >>>>> >>>>> unsigned long pgsize_bitmap; >>>>> >>>>> And add documentation for the error codes there. >>>>> >>>>> If you want to ignore some errors returned from the add_device operation >>>>> you should document it similarly, and in particular document which error >>>>> check(s) need to be performed by of_xlate and which are the >>>>> responsibility of add_device. >>>>> >>>>>> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with >>>>>> deferred probing or error") >>>>>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> >>>>>> Tested-by: Magnus Damn <magnus.damn@gmail.com> >>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>>>> --- >>>>>> [V2] Corrected spelling/case in commit log >>>>>> >>>>>> drivers/iommu/of_iommu.c | 6 ++++++ >>>>>> 1 file changed, 6 insertions(+) >>>>>> >>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >>>>>> index e6e9bec..f0d22c0 100644 >>>>>> --- a/drivers/iommu/of_iommu.c >>>>>> +++ b/drivers/iommu/of_iommu.c >>>>>> @@ -237,6 +237,12 @@ const struct iommu_ops *of_iommu_configure(struct >>>>>> device *dev, >>>>>> ops = ERR_PTR(err); >>>>>> } >>>>>> >>>>>> + /* Ignore all other errors apart from EPROBE_DEFER */ >>>>>> + if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) { >>>>>> + dev_info(dev, "Adding to IOMMU failed: %ld\n", > PTR_ERR(ops)); > > I would turn this into a dev_dbg(), as at least the -ENODEV error returned > from add_device has a defined meaning (see the comment in add_iommu_group()) > and is in my opinion not a condition that should be reported in the kernel log > by default. > ok, will turn this in to a dev_dbg. Regards, Sricharan >>>>>> + ops = NULL; >>>>>> + } >>>>>> + >>>>>> return ops; >>>>>> } >
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 2cb54adc4a33..6ba553e7384a 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -181,7 +181,6 @@ struct iommu_resv_region { * @domain_window_disable: Disable a particular window for a domain * @domain_set_windows: Set the number of windows for a domain * @domain_get_windows: Return the number of windows for a domain - * @of_xlate: add OF master IDs to iommu grouping * @pgsize_bitmap: bitmap of all possible supported page sizes */ struct iommu_ops { @@ -224,6 +223,11 @@ struct iommu_ops { /* Get the number of windows per domain */ u32 (*domain_get_windows)(struct iommu_domain *domain); + /** + * @of_xlate: + * + * Add OF master IDs to iommu grouping. + */ int (*of_xlate)(struct device *dev, struct of_phandle_args *args); unsigned long pgsize_bitmap;