Re: [PATCH net-next 3/6] net: ipa: move and redefine ipa_version_valid()

[Alex Elder]

On 9/20/22 3:29 AM, Paolo Abeni wrote:
> On Fri, 2022-09-09 at 20:11 -0500, Alex Elder wrote:
> > Move the definition of ipa_version_valid(), making it a static
> > inline function defined together with the enumerated type in
> > "ipa_version.h".  Define a new count value in the type.
> >
> > Rename the function to be ipa_version_supported(), and have it
> > return true only if the IPA version supplied is explicitly supported
> > by the driver.
>
> I'm wondering if the above is going to cause regressions with some IPA
> versions suddenly not probed anymore by the module?

That is a really good observation.

The way versions are handled is a little bit inconsistent.  The
code is generally written in such a way that *any* version could
be used (between a certain minimum and maximum, currently 3.0-4.11).
In other words, the *intent* in the code is to make it so that
quirks and features that are version-specific are handled the right
way, even if we do not (yet) support it.

So for example the inline macro rsrc_grp_encoded() returns the
mask to use to specify an endpoint's assigned resource group.
IPA v4.7 uses one bit, whereas others use two or three bits.
We don't "formally" support IPA v4.7, because I (or someone
else) haven't set up a Device Tree file and "IPA config data"
to test it on real hardware.  Still, rsrc_grp_encoded() returns
the right value for IPA v4.7, even though it won't be needed
until IPA v4.7 is tested and declared supported.

The intent is to facilitate adding support for IPA v4.7 (and
others).  In principle one could simply try it out and it should
work, but in reality it is unlikely to be that easy.

Finally, as mentioned, to support a version (such as 4.7) we
need to create "ipa_data-v4.7.c", which defines a bunch of
things that are version-specific.  Because those definitions
are missing, no IPA v4.7 hardware will be matched by the
ipa_match[] table.

So the answer to your question is that currently none of the
unsupported versions will successfully probe anyway.

> Additionally there are a few places checking for the now unsupported
> version[s], I guess that check could/should be removed? e.g.
> ipa_reg_irq_suspend_en_ee_n_offset(),
> ipa_reg_irq_suspend_info_ee_n_offset()
> ...

I'm a fan of removing unused code like this, but I really would
like to actually support these other IPA versions, and I hope
the code is close to ready for that.  I would just need to get
some hardware to test it with (and it needs to rise to the top
of my priority list...).

Does this make sense to you?

Thank you very much for taking the time to review this.

					-Alex

> Thanks,
>
> Paolo