Re: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

Re: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Randy Dunlap]

On 4/8/21 2:15 AM, Dexuan Cui wrote:
> diff --git a/drivers/net/ethernet/microsoft/Kconfig b/drivers/net/ethernet/microsoft/Kconfig
> new file mode 100644
> index 000000000000..12ef6b581566
> --- /dev/null
> +++ b/drivers/net/ethernet/microsoft/Kconfig
> @@ -0,0 +1,30 @@
> +#
> +# Microsoft Azure network device configuration
> +#
> +
> +config NET_VENDOR_MICROSOFT
> +	bool "Microsoft Azure Network Device"

Seems to me that should be generalized, more like:

	bool "Microsoft Network Devices"


> +	default y
> +	help
> +	  If you have a network (Ethernet) device belonging to this class, say Y.
> +
> +	  Note that the answer to this question doesn't directly affect the
> +	  kernel: saying N will just cause the configurator to skip the
> +	  question about Microsoft Azure network device. If you say Y, you

	           about Microsoft networking devices.

> +	  will be asked for your specific device in the following question.
> +
> +if NET_VENDOR_MICROSOFT
> +
> +config MICROSOFT_MANA
> +	tristate "Microsoft Azure Network Adapter (MANA) support"
> +	default m

Please drop the default m. We don't randomly add drivers to be built.

Or leave this as is and change NET_VENDOR_MICROSOFT to be default n.


> +	depends on PCI_MSI && X86_64
> +	select PCI_HYPERV
> +	help
> +	  This driver supports Microsoft Azure Network Adapter (MANA).
> +	  So far, the driver is only validated on X86_64.

validated how?


> +
> +	  To compile this driver as a module, choose M here.
> +	  The module will be called mana.
> +
> +endif #NET_VENDOR_MICROSOFT


thanks.
-- 
~Randy

RE: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Haiyang Zhang]

> -----Original Message-----
> From: Randy Dunlap <rdunlap@infradead.org>
> Sent: Thursday, April 8, 2021 12:23 PM
> To: Dexuan Cui <decui@microsoft.com>; davem@davemloft.net;
> kuba@kernel.org; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; wei.liu@kernel.org; Wei Liu
> <liuwe@microsoft.com>; netdev@vger.kernel.org; leon@kernel.org;
> andrew@lunn.ch; bernd@petrovitsch.priv.at
> Cc: linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org
> Subject: Re: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure
> Network Adapter (MANA)
> 
> On 4/8/21 2:15 AM, Dexuan Cui wrote:
> > diff --git a/drivers/net/ethernet/microsoft/Kconfig
> b/drivers/net/ethernet/microsoft/Kconfig
> > new file mode 100644
> > index 000000000000..12ef6b581566
> > --- /dev/null
> > +++ b/drivers/net/ethernet/microsoft/Kconfig
> > @@ -0,0 +1,30 @@
> > +#
> > +# Microsoft Azure network device configuration
> > +#
> > +
> > +config NET_VENDOR_MICROSOFT
> > +	bool "Microsoft Azure Network Device"
> 
> Seems to me that should be generalized, more like:
> 
> 	bool "Microsoft Network Devices"
This device is planned for Azure cloud at this time.
We will update the wording if things change.

> 
> 
> > +	default y
> > +	help
> > +	  If you have a network (Ethernet) device belonging to this class, say Y.
> > +
> > +	  Note that the answer to this question doesn't directly affect the
> > +	  kernel: saying N will just cause the configurator to skip the
> > +	  question about Microsoft Azure network device. If you say Y, you
> 
> 	           about Microsoft networking devices.
(ditto)

> 
> > +	  will be asked for your specific device in the following question.
> > +
> > +if NET_VENDOR_MICROSOFT
> > +
> > +config MICROSOFT_MANA
> > +	tristate "Microsoft Azure Network Adapter (MANA) support"
> > +	default m
> 
> Please drop the default m. We don't randomly add drivers to be built.
We will.

> 
> Or leave this as is and change NET_VENDOR_MICROSOFT to be default n.
> 
> 
> > +	depends on PCI_MSI && X86_64
> > +	select PCI_HYPERV
> > +	help
> > +	  This driver supports Microsoft Azure Network Adapter (MANA).
> > +	  So far, the driver is only validated on X86_64.
> 
> validated how?
On our pre-released HW.

Thanks,
- Haiyang

Re: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Andrew Lunn]

> > > diff --git a/drivers/net/ethernet/microsoft/Kconfig
> > b/drivers/net/ethernet/microsoft/Kconfig
> > > new file mode 100644
> > > index 000000000000..12ef6b581566
> > > --- /dev/null
> > > +++ b/drivers/net/ethernet/microsoft/Kconfig
> > > @@ -0,0 +1,30 @@
> > > +#
> > > +# Microsoft Azure network device configuration
> > > +#
> > > +
> > > +config NET_VENDOR_MICROSOFT
> > > +	bool "Microsoft Azure Network Device"
> > 
> > Seems to me that should be generalized, more like:
> > 
> > 	bool "Microsoft Network Devices"
> This device is planned for Azure cloud at this time.
> We will update the wording if things change.

This section is about the Vendor. Broadcom, Marvell, natsemi, toshiba,
etc. Microsoft is the Vendor here and all Microsoft Ethernet drivers
belong here. It does not matter what platform they are for.

       Andrew

RE: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Haiyang Zhang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, April 8, 2021 12:45 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>; Dexuan Cui
> <decui@microsoft.com>; davem@davemloft.net; kuba@kernel.org; KY
> Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; wei.liu@kernel.org; Wei Liu
> <liuwe@microsoft.com>; netdev@vger.kernel.org; leon@kernel.org;
> bernd@petrovitsch.priv.at; linux-kernel@vger.kernel.org; linux-
> hyperv@vger.kernel.org
> Subject: Re: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure
> Network Adapter (MANA)
> 
> > > > diff --git a/drivers/net/ethernet/microsoft/Kconfig
> > > b/drivers/net/ethernet/microsoft/Kconfig
> > > > new file mode 100644
> > > > index 000000000000..12ef6b581566
> > > > --- /dev/null
> > > > +++ b/drivers/net/ethernet/microsoft/Kconfig
> > > > @@ -0,0 +1,30 @@
> > > > +#
> > > > +# Microsoft Azure network device configuration
> > > > +#
> > > > +
> > > > +config NET_VENDOR_MICROSOFT
> > > > +	bool "Microsoft Azure Network Device"
> > >
> > > Seems to me that should be generalized, more like:
> > >
> > > 	bool "Microsoft Network Devices"
> > This device is planned for Azure cloud at this time.
> > We will update the wording if things change.
> 
> This section is about the Vendor. Broadcom, Marvell, natsemi, toshiba,
> etc. Microsoft is the Vendor here and all Microsoft Ethernet drivers
> belong here. It does not matter what platform they are for.

Thanks. We will update the wording.

- Haiyang

Re: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Stephen Hemminger]

On Thu, 8 Apr 2021 09:22:57 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:

> On 4/8/21 2:15 AM, Dexuan Cui wrote:
> > diff --git a/drivers/net/ethernet/microsoft/Kconfig b/drivers/net/ethernet/microsoft/Kconfig
> > new file mode 100644
> > index 000000000000..12ef6b581566
> > --- /dev/null
> > +++ b/drivers/net/ethernet/microsoft/Kconfig
> > @@ -0,0 +1,30 @@
> > +#
> > +# Microsoft Azure network device configuration
> > +#
> > +
> > +config NET_VENDOR_MICROSOFT
> > +	bool "Microsoft Azure Network Device"  
> 
> Seems to me that should be generalized, more like:
> 
> 	bool "Microsoft Network Devices"

Yes, that is what it should be at this level.

> 
> 
> > +	default y

This follows the existing policy for network vendor level

> > +	help
> > +	  If you have a network (Ethernet) device belonging to this class, say Y.
> > +
> > +	  Note that the answer to this question doesn't directly affect the
> > +	  kernel: saying N will just cause the configurator to skip the
> > +	  question about Microsoft Azure network device. If you say Y, you  
> 
> 	           about Microsoft networking devices.
> 
> > +	  will be asked for your specific device in the following question.
> > +
> > +if NET_VENDOR_MICROSOFT
> > +
> > +config MICROSOFT_MANA
> > +	tristate "Microsoft Azure Network Adapter (MANA) support"
> > +	default m  
> 
> Please drop the default m. We don't randomly add drivers to be built.

Yes, it should be no (or no default which is the default for default)

> Or leave this as is and change NET_VENDOR_MICROSOFT to be default n.
> 
> 
> > +	depends on PCI_MSI && X86_64
> > +	select PCI_HYPERV
> > +	help
> > +	  This driver supports Microsoft Azure Network Adapter (MANA).
> > +	  So far, the driver is only validated on X86_64.  
> 
> validated how?

Maybe change validated to supported?

RE: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Haiyang Zhang]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, April 8, 2021 12:52 PM
> To: Randy Dunlap <rdunlap@infradead.org>
> Cc: Dexuan Cui <decui@microsoft.com>; davem@davemloft.net;
> kuba@kernel.org; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; wei.liu@kernel.org; Wei Liu
> <liuwe@microsoft.com>; netdev@vger.kernel.org; leon@kernel.org;
> andrew@lunn.ch; bernd@petrovitsch.priv.at; linux-kernel@vger.kernel.org;
> linux-hyperv@vger.kernel.org
> Subject: Re: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure
> Network Adapter (MANA)
> 
> On Thu, 8 Apr 2021 09:22:57 -0700
> Randy Dunlap <rdunlap@infradead.org> wrote:
> 
> > On 4/8/21 2:15 AM, Dexuan Cui wrote:
> > > diff --git a/drivers/net/ethernet/microsoft/Kconfig
> > > b/drivers/net/ethernet/microsoft/Kconfig
> > > new file mode 100644
> > > index 000000000000..12ef6b581566
> > > --- /dev/null
> > > +++ b/drivers/net/ethernet/microsoft/Kconfig
> > > @@ -0,0 +1,30 @@
> > > +#
> > > +# Microsoft Azure network device configuration #
> > > +
> > > +config NET_VENDOR_MICROSOFT
> > > +	bool "Microsoft Azure Network Device"
> >
> > Seems to me that should be generalized, more like:
> >
> > 	bool "Microsoft Network Devices"
> 
> Yes, that is what it should be at this level.
> 
> >
> >
> > > +	default y
> 
> This follows the existing policy for network vendor level
> 
> > > +	help
> > > +	  If you have a network (Ethernet) device belonging to this class, say Y.
> > > +
> > > +	  Note that the answer to this question doesn't directly affect the
> > > +	  kernel: saying N will just cause the configurator to skip the
> > > +	  question about Microsoft Azure network device. If you say Y, you
> >
> > 	           about Microsoft networking devices.
> >
> > > +	  will be asked for your specific device in the following question.
> > > +
> > > +if NET_VENDOR_MICROSOFT
> > > +
> > > +config MICROSOFT_MANA
> > > +	tristate "Microsoft Azure Network Adapter (MANA) support"
> > > +	default m
> >
> > Please drop the default m. We don't randomly add drivers to be built.
> 
> Yes, it should be no (or no default which is the default for default)
> 
> > Or leave this as is and change NET_VENDOR_MICROSOFT to be default n.
> >
> >
> > > +	depends on PCI_MSI && X86_64
> > > +	select PCI_HYPERV
> > > +	help
> > > +	  This driver supports Microsoft Azure Network Adapter (MANA).
> > > +	  So far, the driver is only validated on X86_64.
> >
> > validated how?
> 
> Maybe change validated to supported?

Sounds better. We will change it to "supported".
Also other suggested changes.

Thanks,
- Haiyang

RE: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dexuan Cui]

> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, April 8, 2021 9:52 AM

Thanks all for your input! We'll make the below changes as suggested:

Microsoft Azure Network Device ==> Microsoft Network Devices
Drop the default m
validated ==> supported

We'll also fix some warnings reported by "kernel test robot".

Will post v3 later today.

Thanks,
Dexuan

diff --git a/drivers/net/ethernet/microsoft/Kconfig b/drivers/net/ethernet/microsoft/Kconfig
index 12ef6b581566..e1ac0a5d808d 100644
--- a/drivers/net/ethernet/microsoft/Kconfig
+++ b/drivers/net/ethernet/microsoft/Kconfig
@@ -3,26 +3,25 @@
 #
 
 config NET_VENDOR_MICROSOFT
-	bool "Microsoft Azure Network Device"
+	bool "Microsoft Network Devices"
 	default y
 	help
 	  If you have a network (Ethernet) device belonging to this class, say Y.
 
 	  Note that the answer to this question doesn't directly affect the
 	  kernel: saying N will just cause the configurator to skip the
-	  question about Microsoft Azure network device. If you say Y, you
-	  will be asked for your specific device in the following question.
+	  question about Microsoft network devices. If you say Y, you will be
+	  asked for your specific device in the following question.
 
 if NET_VENDOR_MICROSOFT
 
 config MICROSOFT_MANA
 	tristate "Microsoft Azure Network Adapter (MANA) support"
-	default m
 	depends on PCI_MSI && X86_64
 	select PCI_HYPERV
 	help
 	  This driver supports Microsoft Azure Network Adapter (MANA).
-	  So far, the driver is only validated on X86_64.
+	  So far, the driver is only supported on X86_64.
 
 	  To compile this driver as a module, choose M here.
 	  The module will be called mana.