Message-ID: <20230522144241-mutt-send-email-mst@kernel.org>
Date: Mon, 22 May 2023 15:04:39 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Willem de Bruijn <willemb@...gle.com>
Cc: Shannon Nelson <shannon.nelson@....com>,
	"Samudrala, Sridhar" <sridhar.samudrala@...el.com>,
	Emil Tantilov <emil.s.tantilov@...el.com>,
	intel-wired-lan@...ts.osuosl.org, simon.horman@...igine.com,
	leon@...nel.org, decot@...gle.com, jesse.brandeburg@...el.com,
	anthony.l.nguyen@...el.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	netdev@...r.kernel.org,
	"Singhai, Anjali" <anjali.singhai@...el.com>,
	"Orr, Michael" <michael.orr@...el.com>
Subject: Re: [PATCH iwl-next v4 00/15] Introduce Intel IDPF driver

On Fri, May 19, 2023 at 01:12:43PM -0400, Willem de Bruijn wrote:
> On Fri, May 19, 2023 at 12:17 PM Shannon Nelson <shannon.nelson@....com> wrote:
> >
> > On 5/18/23 4:26 PM, Samudrala, Sridhar wrote:
> > > On 5/18/2023 10:10 AM, Michael S. Tsirkin wrote:
> > >> On Thu, May 18, 2023 at 09:19:31AM -0700, Samudrala, Sridhar wrote:
> > >>>
> > >>>
> > >>> On 5/11/2023 11:34 PM, Michael S. Tsirkin wrote:
> > >>>> On Mon, May 08, 2023 at 12:43:11PM -0700, Emil Tantilov wrote:
> > >>>>> This patch series introduces the Intel Infrastructure Data Path
> > >>>>> Function
> > >>>>> (IDPF) driver. It is used for both physical and virtual functions.
> > >>>>> Except
> > >>>>> for some of the device operations the rest of the functionality is the
> > >>>>> same for both PF and VF. IDPF uses virtchnl version2 opcodes and
> > >>>>> structures defined in the virtchnl2 header file which helps the driver
> > >>>>> to learn the capabilities and register offsets from the device
> > >>>>> Control Plane (CP) instead of assuming the default values.
> > >>>>
> > >>>> So, is this for merge in the next cycle?  Should this be an RFC rather?
> > >>>> It seems unlikely that the IDPF specification will be finalized by that
> > >>>> time - how are you going to handle any specification changes?
> > >>>
> > >>> Yes. we would like this driver to be merged in the next cycle(6.5).
> > >>> Based on the community feedback on v1 version of the driver, we
> > >>> removed all
> > >>> references to OASIS standard and at this time this is an intel vendor
> > >>> driver.
> > >>>
> > >>> Links to v1 and v2 discussion threads
> > >>> https://lore.kernel.org/netdev/20230329140404.1647925-1-pavan.kumar.linga@intel.com/
> > >>> https://lore.kernel.org/netdev/20230411011354.2619359-1-pavan.kumar.linga@intel.com/
> > >>>
> > >>> The v1->v2 change log reflects this update.
> > >>> v1 --> v2: link [1]
> > >>>   * removed the OASIS reference in the commit message to make it clear
> > >>>     that this is an Intel vendor specific driver
> > >>
> > >> Yes this makes sense.
> > >>
> > >>
> > >>> Any IDPF specification updates would be handled as part of the
> > >>> changes that
> > >>> would be required to make this a common standards driver.
> > >>
> > >>
> > >> So my question is, would it make sense to update Kconfig and module name
> > >> to be "ipu" or if you prefer "intel-idpf" to make it clear this is
> > >> currently an Intel vendor specific driver?  And then when you make it a
> > >> common standards driver rename it to idpf?  The point being to help make
> > >> sure users are not confused about whether they got a driver with
> > >> or without IDPF updates. It's not critical I guess but seems like a good
> > >> idea. WDYT?
> > >
> > > It would be more disruptive to change the name of the driver. We can
> > > update the pci device table, module description and possibly driver
> > > version when we are ready to make this a standard driver.
> > > So we would prefer not changing the driver name.
> >
> > More disruptive for who?
> >
> > I think it would be better to change the name of the one driver now
> > before a problem is created in the tree than to leave a point of
> > confusion for the rest of the drivers to contend with in the future.
> 
> This discussion is premised on the idea that the drivers will
> inevitably fork, with an Intel driver and a non-backward compatible
> standardized driver.
> 
> Instead, I expect that the goal is that the future standardized driver
> will iterate and support additional features. But that the existing
> hardware will continue to be supported, if perhaps with updated
> firmware.
> 
> IDPF from the start uses feature negotiation over virtchannel to be
> highly configurable. A future driver might deprecate older feature
> (variants), while either still continue to support those or require
> firmware updates to match the new version.
> 
> Even if the device API would break in a non-backward compatible way,
> the same driver can support both versions. Virtio is an example of
> this.
> 
> If I'm wrong and for some reason two drivers would have to be
> supported, then I'm sure we can figure out a suffix or prefix to the
> standard driver that separates it from the existing one.


OK let us look at a more specific example.

The IDPF TC recently voted to bind to device
based on class/programming interface as opposed to device/vendor id.
Future driver will likely do this. Current driver only binds to intel's
device and vendor id. Assuming this happens, what bothers me is that
depending on kernel version, driver idpf.ko either does or does not bind
to idpf programming interface.

All this is quite imminent.

Yes tricks like checking module version to check what is supported is
possible, but we are beginning to already develop technical dept and
lore and we just started. Simple lsmod|grep -q idpf seems like an
easier, more robust and intuitive way than if [[ $(cat
/sys/module/idpf/version) == "2.0" ]] then echo "idpf" fi

And yes somemore might ship the existing driver
during this initial window. Then we get
lsmod|grep -q -e idpf -e intel-ipu
still seems pretty clean. And hopefully the change will happen within
a couple of months so not more than one release will have
the intel-ipu name.

Of course all this is not earth shatteringly important but still,
I'm interested in what others think.

-- 
MST

Powered by blists - more mailing lists