Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
03.10.2020 02:53, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 11:12:18PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 22:45, Nicolin Chen пишет:
>>> On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
02.10.2020 09:08, Nicolin Chen пишет:
> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>struct device *dev)
> {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> struct tegra_smmu_as *as = to_smmu_as(domain);
> - struct device_node *np = dev->of_node;
> - struct of_phandle_args args;
> unsigned int index = 0;
> int err = 0;
>
> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> -)) {
> - unsigned int swgroup = args.args[0];
> -
> - if (args.np != smmu->dev->of_node) {
> - of_node_put(args.np);
> - continue;
> - }
> -
> - of_node_put(args.np);
> + if (!fwspec)
> + return -ENOENT;
Could the !fwspec ever be true here as well?
>>>
>>> There are multiple callers of this function. It's really not that
>>> straightforward to track every one of them. So I'd rather have it
>>> here as other iommu drivers do. We are human beings, so we could
>>> have missed something somewhere, especially callers are not from
>>> tegra-* drivers.
>>>
>>
>> I'm looking at the IOMMU core and it requires device to be in IOMMU
>> group before attach_dev() could be called.
>>
>> The group can't be assigned to device without the fwspec, see
>> tegra_smmu_device_group().
>>
>> Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
>> NULL in attach_dev(), some not checking anything, some check both and
>> only arm-smmu checks the fwspec.
>
> As I said a couple of days ago, I don't like to assume that the
> callers won't change. And this time, it's from open code. So I
> don't want to assume that there won't be a change.
>
> If you are confident that there is no need to add such a check,
> please send patches to remove those checks in those drivers to
> see if others would agree. I would be willing to remove it after
> that. Otherwise, I'd like to keep this.
>
> Thanks for the review.
>
I haven't tried to check every code path very thoroughly, expecting you
to do it since you're making this patch. Maybe there is a real reason
why majority of drivers do the checks and it would be good to know why.
Although, it's not critical in this particular case and indeed the
checks could be improved later on.
It looks to me that at least will be a bit better/cleaner to check the
dev_iommu_priv_get() for NULL instead of fwspec because the private
variable depends on the fwspec presence and there is a similar check in
probe_device, hence checks will be more consistent.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
On Fri, Oct 02, 2020 at 11:12:18PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 22:45, Nicolin Chen пишет:
> > On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
> >> 02.10.2020 09:08, Nicolin Chen пишет:
> >>> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
> >>>struct device *dev)
> >>> {
> >>> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >>> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> >>> struct tegra_smmu_as *as = to_smmu_as(domain);
> >>> - struct device_node *np = dev->of_node;
> >>> - struct of_phandle_args args;
> >>> unsigned int index = 0;
> >>> int err = 0;
> >>>
> >>> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> >>> -)) {
> >>> - unsigned int swgroup = args.args[0];
> >>> -
> >>> - if (args.np != smmu->dev->of_node) {
> >>> - of_node_put(args.np);
> >>> - continue;
> >>> - }
> >>> -
> >>> - of_node_put(args.np);
> >>> + if (!fwspec)
> >>> + return -ENOENT;
> >>
> >> Could the !fwspec ever be true here as well?
> >
> > There are multiple callers of this function. It's really not that
> > straightforward to track every one of them. So I'd rather have it
> > here as other iommu drivers do. We are human beings, so we could
> > have missed something somewhere, especially callers are not from
> > tegra-* drivers.
> >
>
> I'm looking at the IOMMU core and it requires device to be in IOMMU
> group before attach_dev() could be called.
>
> The group can't be assigned to device without the fwspec, see
> tegra_smmu_device_group().
>
> Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
> NULL in attach_dev(), some not checking anything, some check both and
> only arm-smmu checks the fwspec.
As I said a couple of days ago, I don't like to assume that the
callers won't change. And this time, it's from open code. So I
don't want to assume that there won't be a change.
If you are confident that there is no need to add such a check,
please send patches to remove those checks in those drivers to
see if others would agree. I would be willing to remove it after
that. Otherwise, I'd like to keep this.
Thanks for the review.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
02.10.2020 22:45, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>>> struct device *dev)
>>> {
>>> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>> struct tegra_smmu_as *as = to_smmu_as(domain);
>>> - struct device_node *np = dev->of_node;
>>> - struct of_phandle_args args;
>>> unsigned int index = 0;
>>> int err = 0;
>>>
>>> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>> - )) {
>>> - unsigned int swgroup = args.args[0];
>>> -
>>> - if (args.np != smmu->dev->of_node) {
>>> - of_node_put(args.np);
>>> - continue;
>>> - }
>>> -
>>> - of_node_put(args.np);
>>> + if (!fwspec)
>>> + return -ENOENT;
>>
>> Could the !fwspec ever be true here as well?
>
> There are multiple callers of this function. It's really not that
> straightforward to track every one of them. So I'd rather have it
> here as other iommu drivers do. We are human beings, so we could
> have missed something somewhere, especially callers are not from
> tegra-* drivers.
>
I'm looking at the IOMMU core and it requires device to be in IOMMU
group before attach_dev() could be called.
The group can't be assigned to device without the fwspec, see
tegra_smmu_device_group().
Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
NULL in attach_dev(), some not checking anything, some check both and
only arm-smmu checks the fwspec.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
On Fri, Oct 02, 2020 at 05:52:00PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 17:22, Dmitry Osipenko пишет:
> > 02.10.2020 09:08, Nicolin Chen пишет:
> >> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
> >> struct device *dev)
> >> {
> >> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >>struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> >>struct tegra_smmu_as *as = to_smmu_as(domain);
> >> - struct device_node *np = dev->of_node;
> >> - struct of_phandle_args args;
> >>unsigned int index = 0;
> >>int err = 0;
> >
> > Looks like there is no need to initialize 'index' and 'err' variables
> > anymore.
> >
>
> Same for tegra_smmu_detach_dev().
Can remove them.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> > static int tegra_smmu_attach_dev(struct iommu_domain *domain,
> > struct device *dev)
> > {
> > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> > struct tegra_smmu_as *as = to_smmu_as(domain);
> > - struct device_node *np = dev->of_node;
> > - struct of_phandle_args args;
> > unsigned int index = 0;
> > int err = 0;
> >
> > - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> > - )) {
> > - unsigned int swgroup = args.args[0];
> > -
> > - if (args.np != smmu->dev->of_node) {
> > - of_node_put(args.np);
> > - continue;
> > - }
> > -
> > - of_node_put(args.np);
> > + if (!fwspec)
> > + return -ENOENT;
>
> Could the !fwspec ever be true here as well?
There are multiple callers of this function. It's really not that
straightforward to track every one of them. So I'd rather have it
here as other iommu drivers do. We are human beings, so we could
have missed something somewhere, especially callers are not from
tegra-* drivers.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
02.10.2020 17:22, Dmitry Osipenko пишет:
> 02.10.2020 09:08, Nicolin Chen пишет:
>> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>> struct device *dev)
>> {
>> +struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>> struct tegra_smmu_as *as = to_smmu_as(domain);
>> -struct device_node *np = dev->of_node;
>> -struct of_phandle_args args;
>> unsigned int index = 0;
>> int err = 0;
>
> Looks like there is no need to initialize 'index' and 'err' variables
> anymore.
>
Same for tegra_smmu_detach_dev().
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
02.10.2020 09:08, Nicolin Chen пишет:
> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>struct device *dev)
> {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> struct tegra_smmu_as *as = to_smmu_as(domain);
> - struct device_node *np = dev->of_node;
> - struct of_phandle_args args;
> unsigned int index = 0;
> int err = 0;
>
> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> -)) {
> - unsigned int swgroup = args.args[0];
> -
> - if (args.np != smmu->dev->of_node) {
> - of_node_put(args.np);
> - continue;
> - }
> -
> - of_node_put(args.np);
> + if (!fwspec)
> + return -ENOENT;
Could the !fwspec ever be true here as well?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
02.10.2020 09:08, Nicolin Chen пишет:
> static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device
> *dev)
> {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct tegra_smmu_as *as = to_smmu_as(domain);
> - struct device_node *np = dev->of_node;
> struct tegra_smmu *smmu = as->smmu;
> - struct of_phandle_args args;
> unsigned int index = 0;
>
> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> -)) {
> - unsigned int swgroup = args.args[0];
> -
> - if (args.np != smmu->dev->of_node) {
> - of_node_put(args.np);
> - continue;
> - }
> -
> - of_node_put(args.np);
> + if (!fwspec)
> + return;
When !fwspec could be true?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
02.10.2020 09:08, Nicolin Chen пишет:
> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>struct device *dev)
> {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> struct tegra_smmu_as *as = to_smmu_as(domain);
> - struct device_node *np = dev->of_node;
> - struct of_phandle_args args;
> unsigned int index = 0;
> int err = 0;
Looks like there is no need to initialize 'index' and 'err' variables
anymore.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
