All of lore.kernel.org
 help / color / mirror / Atom feed
* [net-next 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x)
@ 2020-05-28 15:06 Vadym Kochan
  0 siblings, 0 replies; 7+ messages in thread
From: Vadym Kochan @ 2020-05-28 15:06 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
	Andrew Lunn, Oleksandr Mazur, Serhiy Boiko, Serhiy Pshyk,
	Volodymyr Mytnyk, Taras Chornyi, Andrii Savka, netdev,
	linux-kernel
  Cc: Mickey Rachamim, Vadym Kochan

Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8
ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely
wireless SMB deployment.

Prestera Switchdev is a firmware based driver that operates via PCI bus.  The
current implementation supports only boards designed for the Marvell Switchdev
solution and requires special firmware.

This driver implementation includes only L1, basic L2 support, and RX/TX.

The core Prestera switching logic is implemented in prestera_main.c, there is
an intermediate hw layer between core logic and firmware. It is
implemented in prestera_hw.c, the purpose of it is to encapsulate hw
related logic, in future there is a plan to support more devices with
different HW related configurations.

The following Switchdev features are supported:

    - VLAN-aware bridge offloading
    - VLAN-unaware bridge offloading
    - FDB offloading (learning, ageing)
    - Switchport configuration

The firmware image will be uploaded soon to the linux-firmware repository.

PATCH:
    1) Fixed W=1 warnings

    2) Renamed PCI driver name to be more generic "Prestera DX" because
       there will be more devices supported.

    3) Changed firmware image dir path: marvell/ -> mrvl/prestera/
       to be aligned with location in linux-firmware.git (if such
       will be accepted).

RFC v3:
    1) Fix prestera prefix in prestera_rxtx.c

    2) Protect concurrent access from multiple ports on multiple CPU system
       on tx path by spinlock in prestera_rxtx.c

    3) Try to get base mac address from device-tree, otherwise use a random generated one.

    4) Move ethtool interface support into separate prestera_ethtool.c file.

    5) Add basic devlink support and get rid of physical port naming ops.

    6) Add STP support in Switchdev driver.

    7) Removed MODULE_AUTHOR

    8) Renamed prestera.c -> prestera_main.c, and kernel module to
       prestera.ko

RFC v2:
    1) Use "pestera_" prefix in struct's and functions instead of mvsw_pr_

    2) Original series split into additional patches for Switchdev ethtool support.

    3) Use major and minor firmware version numbers in the firmware image filename.

    4) Removed not needed prints.

    5) Use iopoll API for waiting on register's value in prestera_pci.c

    6) Use standart approach for describing PCI ID matching section instead of using
       custom wrappers in prestera_pci.c

    7) Add RX/TX support in prestera_rxtx.c.

    8) Rewritten prestera_switchdev.c with following changes:
       - handle netdev events from prestera.c

       - use struct prestera_bridge for bridge objects, and get rid of
         struct prestera_bridge_device which may confuse.

       - use refcount_t

    9) Get rid of macro usage for sending fw requests in prestera_hw.c

    10) Add base_mac setting as module parameter. base_mac is required for
        generation default port's mac.

Vadym Kochan (6):
  net: marvell: prestera: Add driver for Prestera family ASIC devices
  net: marvell: prestera: Add PCI interface support
  net: marvell: prestera: Add basic devlink support
  net: marvell: prestera: Add ethtool interface support
  net: marvell: prestera: Add Switchdev driver implementation
  dt-bindings: marvell,prestera: Add description for device-tree
    bindings

 .../bindings/net/marvell,prestera.txt         |   34 +
 drivers/net/ethernet/marvell/Kconfig          |    1 +
 drivers/net/ethernet/marvell/Makefile         |    1 +
 drivers/net/ethernet/marvell/prestera/Kconfig |   25 +
 .../net/ethernet/marvell/prestera/Makefile    |    7 +
 .../net/ethernet/marvell/prestera/prestera.h  |  208 +++
 .../marvell/prestera/prestera_devlink.c       |  111 ++
 .../marvell/prestera/prestera_devlink.h       |   25 +
 .../ethernet/marvell/prestera/prestera_dsa.c  |  134 ++
 .../ethernet/marvell/prestera/prestera_dsa.h  |   37 +
 .../marvell/prestera/prestera_ethtool.c       |  737 ++++++++++
 .../marvell/prestera/prestera_ethtool.h       |   37 +
 .../ethernet/marvell/prestera/prestera_hw.c   | 1225 ++++++++++++++++
 .../ethernet/marvell/prestera/prestera_hw.h   |  180 +++
 .../ethernet/marvell/prestera/prestera_main.c |  663 +++++++++
 .../ethernet/marvell/prestera/prestera_pci.c  |  825 +++++++++++
 .../ethernet/marvell/prestera/prestera_rxtx.c |  860 +++++++++++
 .../ethernet/marvell/prestera/prestera_rxtx.h |   21 +
 .../marvell/prestera/prestera_switchdev.c     | 1286 +++++++++++++++++
 .../marvell/prestera/prestera_switchdev.h     |   16 +
 20 files changed, 6433 insertions(+)
 create mode 100644 drivers/net/ethernet/marvell/prestera/Kconfig
 create mode 100644 drivers/net/ethernet/marvell/prestera/Makefile
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_main.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_pci.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net-next 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x)
  2020-06-01  6:24       ` Jiri Pirko
@ 2020-06-20 13:25         ` Vadym Kochan
  0 siblings, 0 replies; 7+ messages in thread
From: Vadym Kochan @ 2020-06-20 13:25 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Ido Schimmel, David S. Miller, Jakub Kicinski, Jiri Pirko,
	Ido Schimmel, Andrew Lunn, Oleksandr Mazur, Serhiy Boiko,
	Serhiy Pshyk, Volodymyr Mytnyk, Taras Chornyi, Andrii Savka,
	netdev, linux-kernel, Mickey Rachamim

Hi Jiri, Ido,

On Mon, Jun 01, 2020 at 08:24:17AM +0200, Jiri Pirko wrote:
> Sat, May 30, 2020 at 05:54:29PM CEST, idosch@idosch.org wrote:
> >On Sat, May 30, 2020 at 05:52:31PM +0300, Vadym Kochan wrote:
> 
> [...]
> 
> 
> >> > WARNING: do not add new typedefs
> >> > #1064: FILE: drivers/net/ethernet/marvell/prestera/prestera_hw.h:32:
> >> > +typedef void (*prestera_event_cb_t)
> >> I may be wrong, as I remember Jiri suggested it and looks like
> >> it makes sense. I really don't have strong opinion about this.
> >
> >OK, so I'll let Jiri comment when he is back at work.
> 
> I was not aware of this warning, but for function callbacks, I think it
> is very handy to have them as typedef instead of repeating the prototype
> over and over. For that, I don't think this warning makes sense.
> 
> [...]

As I said I have no strong opinion on this, but Jiri's suggestion makes
sense. It looks like typedef check was mostly about 'struct' and native
types definition.

Regards,
Vadym Kochan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net-next 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x)
  2020-05-30 15:54     ` Ido Schimmel
@ 2020-06-01  6:24       ` Jiri Pirko
  2020-06-20 13:25         ` Vadym Kochan
  0 siblings, 1 reply; 7+ messages in thread
From: Jiri Pirko @ 2020-06-01  6:24 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Vadym Kochan, David S. Miller, Jakub Kicinski, Jiri Pirko,
	Ido Schimmel, Andrew Lunn, Oleksandr Mazur, Serhiy Boiko,
	Serhiy Pshyk, Volodymyr Mytnyk, Taras Chornyi, Andrii Savka,
	netdev, linux-kernel, Mickey Rachamim

Sat, May 30, 2020 at 05:54:29PM CEST, idosch@idosch.org wrote:
>On Sat, May 30, 2020 at 05:52:31PM +0300, Vadym Kochan wrote:

[...]


>> > WARNING: do not add new typedefs
>> > #1064: FILE: drivers/net/ethernet/marvell/prestera/prestera_hw.h:32:
>> > +typedef void (*prestera_event_cb_t)
>> I may be wrong, as I remember Jiri suggested it and looks like
>> it makes sense. I really don't have strong opinion about this.
>
>OK, so I'll let Jiri comment when he is back at work.

I was not aware of this warning, but for function callbacks, I think it
is very handy to have them as typedef instead of repeating the prototype
over and over. For that, I don't think this warning makes sense.

[...]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net-next 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x)
  2020-05-30 14:52   ` Vadym Kochan
@ 2020-05-30 15:54     ` Ido Schimmel
  2020-06-01  6:24       ` Jiri Pirko
  0 siblings, 1 reply; 7+ messages in thread
From: Ido Schimmel @ 2020-05-30 15:54 UTC (permalink / raw)
  To: Vadym Kochan
  Cc: David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
	Andrew Lunn, Oleksandr Mazur, Serhiy Boiko, Serhiy Pshyk,
	Volodymyr Mytnyk, Taras Chornyi, Andrii Savka, netdev,
	linux-kernel, Mickey Rachamim

On Sat, May 30, 2020 at 05:52:31PM +0300, Vadym Kochan wrote:
> Hi Ido,
> 
> On Sat, May 30, 2020 at 05:29:28PM +0300, Ido Schimmel wrote:
> > On Thu, May 28, 2020 at 06:12:39PM +0300, Vadym Kochan wrote:
> > > Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8
> > > ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely
> > > wireless SMB deployment.
> > > 
> > > Prestera Switchdev is a firmware based driver that operates via PCI bus.  The
> > > current implementation supports only boards designed for the Marvell Switchdev
> > > solution and requires special firmware.
> > > 
> > > This driver implementation includes only L1, basic L2 support, and RX/TX.
> > > 
> > > The core Prestera switching logic is implemented in prestera_main.c, there is
> > > an intermediate hw layer between core logic and firmware. It is
> > > implemented in prestera_hw.c, the purpose of it is to encapsulate hw
> > > related logic, in future there is a plan to support more devices with
> > > different HW related configurations.
> > > 
> > > The following Switchdev features are supported:
> > > 
> > >     - VLAN-aware bridge offloading
> > >     - VLAN-unaware bridge offloading
> > >     - FDB offloading (learning, ageing)
> > >     - Switchport configuration
> > > 
> > > The firmware image will be uploaded soon to the linux-firmware repository.
> > > 
> > > PATCH:
> > >     1) Fixed W=1 warnings
> > 
> > Hi,
> > 
> > I just applied the patches for review and checkpatch had a lot of
> > complaints. Some are even ERRORs. For example:
> > 
> > WARNING: do not add new typedefs
> > #1064: FILE: drivers/net/ethernet/marvell/prestera/prestera_hw.h:32:
> > +typedef void (*prestera_event_cb_t)
> I may be wrong, as I remember Jiri suggested it and looks like
> it makes sense. I really don't have strong opinion about this.

OK, so I'll let Jiri comment when he is back at work.

> 
> > 
> > WARNING: line over 80 characters
> > #2007: FILE: drivers/net/ethernet/marvell/prestera/prestera_rxtx.c:321:
> > +                       __skb_trim(buf->skb, PRESTERA_SDMA_RX_DESC_PKT_LEN(desc));
> > 
> > WARNING: line over 80 characters
> > #2007: FILE: drivers/net/ethernet/marvell/prestera/prestera_rxtx.c:321:
> > +                       __skb_trim(buf->skb, PRESTERA_SDMA_RX_DESC_PKT_LEN(desc));
> > 
> > ERROR: Macros with complex values should be enclosed in parentheses
> > #196: FILE: drivers/net/ethernet/marvell/prestera/prestera_pci.c:161:
> > +#define PRESTERA_FW_REG_ADDR(fw, reg)  PRESTERA_FW_REG_BASE(fw) + (reg)
> This one makes sense.
> > 
> > WARNING: prefer 'help' over '---help---' for new help texts
> > #52: FILE: drivers/net/ethernet/marvell/prestera/Kconfig:15:
> > +config PRESTERA_PCI
> I will fix it.
> > 
> > ...
> 
> The most are about using ethtool types which are in camel style.
> Regarding > 80 chars is is a required rule ? I saw some discussion
> on LKML that 80+ are acceptable sometimes.

Yea, that's why I didn't include them. Error messages can always exceed
80 characters. Other times I try to follow the rule unless
"the cure is worse than the disease":

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1818701.html

> 
> > 
> > Also, smatch complaints about:
> > 
> > drivers/net/ethernet/marvell/prestera//prestera_ethtool.c:713
> > prestera_ethtool_get_strings() error: memcpy() '*prestera_cnt_name' too
> > small (32 vs 960)
> > 
> > And coccicheck about:
> > 
> > drivers/net/ethernet/marvell/prestera/prestera_hw.c:681:2-3: Unneeded
> > semicolon
> These looks interesting, I did not use smatch and coccicheck, will look
> on these.
> 
> > 
> > > 
> > >     2) Renamed PCI driver name to be more generic "Prestera DX" because
> > >        there will be more devices supported.
> > > 
> > >     3) Changed firmware image dir path: marvell/ -> mrvl/prestera/
> > >        to be aligned with location in linux-firmware.git (if such
> > >        will be accepted).
> > > 
> > > RFC v3:
> > >     1) Fix prestera prefix in prestera_rxtx.c
> > > 
> > >     2) Protect concurrent access from multiple ports on multiple CPU system
> > >        on tx path by spinlock in prestera_rxtx.c
> > > 
> > >     3) Try to get base mac address from device-tree, otherwise use a random generated one.
> > > 
> > >     4) Move ethtool interface support into separate prestera_ethtool.c file.
> > > 
> > >     5) Add basic devlink support and get rid of physical port naming ops.
> > > 
> > >     6) Add STP support in Switchdev driver.
> > > 
> > >     7) Removed MODULE_AUTHOR
> > > 
> > >     8) Renamed prestera.c -> prestera_main.c, and kernel module to
> > >        prestera.ko
> > > 
> > > RFC v2:
> > >     1) Use "pestera_" prefix in struct's and functions instead of mvsw_pr_
> > > 
> > >     2) Original series split into additional patches for Switchdev ethtool support.
> > > 
> > >     3) Use major and minor firmware version numbers in the firmware image filename.
> > > 
> > >     4) Removed not needed prints.
> > > 
> > >     5) Use iopoll API for waiting on register's value in prestera_pci.c
> > > 
> > >     6) Use standart approach for describing PCI ID matching section instead of using
> > >        custom wrappers in prestera_pci.c
> > > 
> > >     7) Add RX/TX support in prestera_rxtx.c.
> > > 
> > >     8) Rewritten prestera_switchdev.c with following changes:
> > >        - handle netdev events from prestera.c
> > > 
> > >        - use struct prestera_bridge for bridge objects, and get rid of
> > >          struct prestera_bridge_device which may confuse.
> > > 
> > >        - use refcount_t
> > > 
> > >     9) Get rid of macro usage for sending fw requests in prestera_hw.c
> > > 
> > >     10) Add base_mac setting as module parameter. base_mac is required for
> > >         generation default port's mac.
> > > 
> > > Vadym Kochan (6):
> > >   net: marvell: prestera: Add driver for Prestera family ASIC devices
> > >   net: marvell: prestera: Add PCI interface support
> > >   net: marvell: prestera: Add basic devlink support
> > >   net: marvell: prestera: Add ethtool interface support
> > >   net: marvell: prestera: Add Switchdev driver implementation
> > >   dt-bindings: marvell,prestera: Add description for device-tree
> > >     bindings
> > > 
> > >  .../bindings/net/marvell,prestera.txt         |   34 +
> > >  drivers/net/ethernet/marvell/Kconfig          |    1 +
> > >  drivers/net/ethernet/marvell/Makefile         |    1 +
> > >  drivers/net/ethernet/marvell/prestera/Kconfig |   25 +
> > >  .../net/ethernet/marvell/prestera/Makefile    |    7 +
> > >  .../net/ethernet/marvell/prestera/prestera.h  |  208 +++
> > >  .../marvell/prestera/prestera_devlink.c       |  111 ++
> > >  .../marvell/prestera/prestera_devlink.h       |   25 +
> > >  .../ethernet/marvell/prestera/prestera_dsa.c  |  134 ++
> > >  .../ethernet/marvell/prestera/prestera_dsa.h  |   37 +
> > >  .../marvell/prestera/prestera_ethtool.c       |  737 ++++++++++
> > >  .../marvell/prestera/prestera_ethtool.h       |   37 +
> > >  .../ethernet/marvell/prestera/prestera_hw.c   | 1225 ++++++++++++++++
> > >  .../ethernet/marvell/prestera/prestera_hw.h   |  180 +++
> > >  .../ethernet/marvell/prestera/prestera_main.c |  663 +++++++++
> > >  .../ethernet/marvell/prestera/prestera_pci.c  |  825 +++++++++++
> > >  .../ethernet/marvell/prestera/prestera_rxtx.c |  860 +++++++++++
> > >  .../ethernet/marvell/prestera/prestera_rxtx.h |   21 +
> > >  .../marvell/prestera/prestera_switchdev.c     | 1286 +++++++++++++++++
> > >  .../marvell/prestera/prestera_switchdev.h     |   16 +
> > >  20 files changed, 6433 insertions(+)
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/Kconfig
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/Makefile
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.h
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.c
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.h
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.c
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.h
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.h
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.h
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_main.c
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_pci.c
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.h
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
> > >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.h
> > > 
> > > -- 
> > > 2.17.1
> > > 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net-next 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x)
  2020-05-30 14:29 ` Ido Schimmel
@ 2020-05-30 14:52   ` Vadym Kochan
  2020-05-30 15:54     ` Ido Schimmel
  0 siblings, 1 reply; 7+ messages in thread
From: Vadym Kochan @ 2020-05-30 14:52 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
	Andrew Lunn, Oleksandr Mazur, Serhiy Boiko, Serhiy Pshyk,
	Volodymyr Mytnyk, Taras Chornyi, Andrii Savka, netdev,
	linux-kernel, Mickey Rachamim

Hi Ido,

On Sat, May 30, 2020 at 05:29:28PM +0300, Ido Schimmel wrote:
> On Thu, May 28, 2020 at 06:12:39PM +0300, Vadym Kochan wrote:
> > Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8
> > ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely
> > wireless SMB deployment.
> > 
> > Prestera Switchdev is a firmware based driver that operates via PCI bus.  The
> > current implementation supports only boards designed for the Marvell Switchdev
> > solution and requires special firmware.
> > 
> > This driver implementation includes only L1, basic L2 support, and RX/TX.
> > 
> > The core Prestera switching logic is implemented in prestera_main.c, there is
> > an intermediate hw layer between core logic and firmware. It is
> > implemented in prestera_hw.c, the purpose of it is to encapsulate hw
> > related logic, in future there is a plan to support more devices with
> > different HW related configurations.
> > 
> > The following Switchdev features are supported:
> > 
> >     - VLAN-aware bridge offloading
> >     - VLAN-unaware bridge offloading
> >     - FDB offloading (learning, ageing)
> >     - Switchport configuration
> > 
> > The firmware image will be uploaded soon to the linux-firmware repository.
> > 
> > PATCH:
> >     1) Fixed W=1 warnings
> 
> Hi,
> 
> I just applied the patches for review and checkpatch had a lot of
> complaints. Some are even ERRORs. For example:
> 
> WARNING: do not add new typedefs
> #1064: FILE: drivers/net/ethernet/marvell/prestera/prestera_hw.h:32:
> +typedef void (*prestera_event_cb_t)
I may be wrong, as I remember Jiri suggested it and looks like
it makes sense. I really don't have strong opinion about this.

> 
> WARNING: line over 80 characters
> #2007: FILE: drivers/net/ethernet/marvell/prestera/prestera_rxtx.c:321:
> +                       __skb_trim(buf->skb, PRESTERA_SDMA_RX_DESC_PKT_LEN(desc));
> 
> WARNING: line over 80 characters
> #2007: FILE: drivers/net/ethernet/marvell/prestera/prestera_rxtx.c:321:
> +                       __skb_trim(buf->skb, PRESTERA_SDMA_RX_DESC_PKT_LEN(desc));
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #196: FILE: drivers/net/ethernet/marvell/prestera/prestera_pci.c:161:
> +#define PRESTERA_FW_REG_ADDR(fw, reg)  PRESTERA_FW_REG_BASE(fw) + (reg)
This one makes sense.
> 
> WARNING: prefer 'help' over '---help---' for new help texts
> #52: FILE: drivers/net/ethernet/marvell/prestera/Kconfig:15:
> +config PRESTERA_PCI
I will fix it.
> 
> ...

The most are about using ethtool types which are in camel style.
Regarding > 80 chars is is a required rule ? I saw some discussion
on LKML that 80+ are acceptable sometimes.

> 
> Also, smatch complaints about:
> 
> drivers/net/ethernet/marvell/prestera//prestera_ethtool.c:713
> prestera_ethtool_get_strings() error: memcpy() '*prestera_cnt_name' too
> small (32 vs 960)
> 
> And coccicheck about:
> 
> drivers/net/ethernet/marvell/prestera/prestera_hw.c:681:2-3: Unneeded
> semicolon
These looks interesting, I did not use smatch and coccicheck, will look
on these.

> 
> > 
> >     2) Renamed PCI driver name to be more generic "Prestera DX" because
> >        there will be more devices supported.
> > 
> >     3) Changed firmware image dir path: marvell/ -> mrvl/prestera/
> >        to be aligned with location in linux-firmware.git (if such
> >        will be accepted).
> > 
> > RFC v3:
> >     1) Fix prestera prefix in prestera_rxtx.c
> > 
> >     2) Protect concurrent access from multiple ports on multiple CPU system
> >        on tx path by spinlock in prestera_rxtx.c
> > 
> >     3) Try to get base mac address from device-tree, otherwise use a random generated one.
> > 
> >     4) Move ethtool interface support into separate prestera_ethtool.c file.
> > 
> >     5) Add basic devlink support and get rid of physical port naming ops.
> > 
> >     6) Add STP support in Switchdev driver.
> > 
> >     7) Removed MODULE_AUTHOR
> > 
> >     8) Renamed prestera.c -> prestera_main.c, and kernel module to
> >        prestera.ko
> > 
> > RFC v2:
> >     1) Use "pestera_" prefix in struct's and functions instead of mvsw_pr_
> > 
> >     2) Original series split into additional patches for Switchdev ethtool support.
> > 
> >     3) Use major and minor firmware version numbers in the firmware image filename.
> > 
> >     4) Removed not needed prints.
> > 
> >     5) Use iopoll API for waiting on register's value in prestera_pci.c
> > 
> >     6) Use standart approach for describing PCI ID matching section instead of using
> >        custom wrappers in prestera_pci.c
> > 
> >     7) Add RX/TX support in prestera_rxtx.c.
> > 
> >     8) Rewritten prestera_switchdev.c with following changes:
> >        - handle netdev events from prestera.c
> > 
> >        - use struct prestera_bridge for bridge objects, and get rid of
> >          struct prestera_bridge_device which may confuse.
> > 
> >        - use refcount_t
> > 
> >     9) Get rid of macro usage for sending fw requests in prestera_hw.c
> > 
> >     10) Add base_mac setting as module parameter. base_mac is required for
> >         generation default port's mac.
> > 
> > Vadym Kochan (6):
> >   net: marvell: prestera: Add driver for Prestera family ASIC devices
> >   net: marvell: prestera: Add PCI interface support
> >   net: marvell: prestera: Add basic devlink support
> >   net: marvell: prestera: Add ethtool interface support
> >   net: marvell: prestera: Add Switchdev driver implementation
> >   dt-bindings: marvell,prestera: Add description for device-tree
> >     bindings
> > 
> >  .../bindings/net/marvell,prestera.txt         |   34 +
> >  drivers/net/ethernet/marvell/Kconfig          |    1 +
> >  drivers/net/ethernet/marvell/Makefile         |    1 +
> >  drivers/net/ethernet/marvell/prestera/Kconfig |   25 +
> >  .../net/ethernet/marvell/prestera/Makefile    |    7 +
> >  .../net/ethernet/marvell/prestera/prestera.h  |  208 +++
> >  .../marvell/prestera/prestera_devlink.c       |  111 ++
> >  .../marvell/prestera/prestera_devlink.h       |   25 +
> >  .../ethernet/marvell/prestera/prestera_dsa.c  |  134 ++
> >  .../ethernet/marvell/prestera/prestera_dsa.h  |   37 +
> >  .../marvell/prestera/prestera_ethtool.c       |  737 ++++++++++
> >  .../marvell/prestera/prestera_ethtool.h       |   37 +
> >  .../ethernet/marvell/prestera/prestera_hw.c   | 1225 ++++++++++++++++
> >  .../ethernet/marvell/prestera/prestera_hw.h   |  180 +++
> >  .../ethernet/marvell/prestera/prestera_main.c |  663 +++++++++
> >  .../ethernet/marvell/prestera/prestera_pci.c  |  825 +++++++++++
> >  .../ethernet/marvell/prestera/prestera_rxtx.c |  860 +++++++++++
> >  .../ethernet/marvell/prestera/prestera_rxtx.h |   21 +
> >  .../marvell/prestera/prestera_switchdev.c     | 1286 +++++++++++++++++
> >  .../marvell/prestera/prestera_switchdev.h     |   16 +
> >  20 files changed, 6433 insertions(+)
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/Kconfig
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/Makefile
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.h
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.c
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.h
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.c
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.h
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.h
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.c
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.h
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_main.c
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_pci.c
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.h
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
> >  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.h
> > 
> > -- 
> > 2.17.1
> > 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net-next 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x)
  2020-05-28 15:12 Vadym Kochan
@ 2020-05-30 14:29 ` Ido Schimmel
  2020-05-30 14:52   ` Vadym Kochan
  0 siblings, 1 reply; 7+ messages in thread
From: Ido Schimmel @ 2020-05-30 14:29 UTC (permalink / raw)
  To: Vadym Kochan
  Cc: David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
	Andrew Lunn, Oleksandr Mazur, Serhiy Boiko, Serhiy Pshyk,
	Volodymyr Mytnyk, Taras Chornyi, Andrii Savka, netdev,
	linux-kernel, Mickey Rachamim

On Thu, May 28, 2020 at 06:12:39PM +0300, Vadym Kochan wrote:
> Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8
> ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely
> wireless SMB deployment.
> 
> Prestera Switchdev is a firmware based driver that operates via PCI bus.  The
> current implementation supports only boards designed for the Marvell Switchdev
> solution and requires special firmware.
> 
> This driver implementation includes only L1, basic L2 support, and RX/TX.
> 
> The core Prestera switching logic is implemented in prestera_main.c, there is
> an intermediate hw layer between core logic and firmware. It is
> implemented in prestera_hw.c, the purpose of it is to encapsulate hw
> related logic, in future there is a plan to support more devices with
> different HW related configurations.
> 
> The following Switchdev features are supported:
> 
>     - VLAN-aware bridge offloading
>     - VLAN-unaware bridge offloading
>     - FDB offloading (learning, ageing)
>     - Switchport configuration
> 
> The firmware image will be uploaded soon to the linux-firmware repository.
> 
> PATCH:
>     1) Fixed W=1 warnings

Hi,

I just applied the patches for review and checkpatch had a lot of
complaints. Some are even ERRORs. For example:

WARNING: do not add new typedefs
#1064: FILE: drivers/net/ethernet/marvell/prestera/prestera_hw.h:32:
+typedef void (*prestera_event_cb_t)

WARNING: line over 80 characters
#2007: FILE: drivers/net/ethernet/marvell/prestera/prestera_rxtx.c:321:
+                       __skb_trim(buf->skb, PRESTERA_SDMA_RX_DESC_PKT_LEN(desc));

WARNING: line over 80 characters
#2007: FILE: drivers/net/ethernet/marvell/prestera/prestera_rxtx.c:321:
+                       __skb_trim(buf->skb, PRESTERA_SDMA_RX_DESC_PKT_LEN(desc));

ERROR: Macros with complex values should be enclosed in parentheses
#196: FILE: drivers/net/ethernet/marvell/prestera/prestera_pci.c:161:
+#define PRESTERA_FW_REG_ADDR(fw, reg)  PRESTERA_FW_REG_BASE(fw) + (reg)

WARNING: prefer 'help' over '---help---' for new help texts
#52: FILE: drivers/net/ethernet/marvell/prestera/Kconfig:15:
+config PRESTERA_PCI

...

Also, smatch complaints about:

drivers/net/ethernet/marvell/prestera//prestera_ethtool.c:713
prestera_ethtool_get_strings() error: memcpy() '*prestera_cnt_name' too
small (32 vs 960)

And coccicheck about:

drivers/net/ethernet/marvell/prestera/prestera_hw.c:681:2-3: Unneeded
semicolon

> 
>     2) Renamed PCI driver name to be more generic "Prestera DX" because
>        there will be more devices supported.
> 
>     3) Changed firmware image dir path: marvell/ -> mrvl/prestera/
>        to be aligned with location in linux-firmware.git (if such
>        will be accepted).
> 
> RFC v3:
>     1) Fix prestera prefix in prestera_rxtx.c
> 
>     2) Protect concurrent access from multiple ports on multiple CPU system
>        on tx path by spinlock in prestera_rxtx.c
> 
>     3) Try to get base mac address from device-tree, otherwise use a random generated one.
> 
>     4) Move ethtool interface support into separate prestera_ethtool.c file.
> 
>     5) Add basic devlink support and get rid of physical port naming ops.
> 
>     6) Add STP support in Switchdev driver.
> 
>     7) Removed MODULE_AUTHOR
> 
>     8) Renamed prestera.c -> prestera_main.c, and kernel module to
>        prestera.ko
> 
> RFC v2:
>     1) Use "pestera_" prefix in struct's and functions instead of mvsw_pr_
> 
>     2) Original series split into additional patches for Switchdev ethtool support.
> 
>     3) Use major and minor firmware version numbers in the firmware image filename.
> 
>     4) Removed not needed prints.
> 
>     5) Use iopoll API for waiting on register's value in prestera_pci.c
> 
>     6) Use standart approach for describing PCI ID matching section instead of using
>        custom wrappers in prestera_pci.c
> 
>     7) Add RX/TX support in prestera_rxtx.c.
> 
>     8) Rewritten prestera_switchdev.c with following changes:
>        - handle netdev events from prestera.c
> 
>        - use struct prestera_bridge for bridge objects, and get rid of
>          struct prestera_bridge_device which may confuse.
> 
>        - use refcount_t
> 
>     9) Get rid of macro usage for sending fw requests in prestera_hw.c
> 
>     10) Add base_mac setting as module parameter. base_mac is required for
>         generation default port's mac.
> 
> Vadym Kochan (6):
>   net: marvell: prestera: Add driver for Prestera family ASIC devices
>   net: marvell: prestera: Add PCI interface support
>   net: marvell: prestera: Add basic devlink support
>   net: marvell: prestera: Add ethtool interface support
>   net: marvell: prestera: Add Switchdev driver implementation
>   dt-bindings: marvell,prestera: Add description for device-tree
>     bindings
> 
>  .../bindings/net/marvell,prestera.txt         |   34 +
>  drivers/net/ethernet/marvell/Kconfig          |    1 +
>  drivers/net/ethernet/marvell/Makefile         |    1 +
>  drivers/net/ethernet/marvell/prestera/Kconfig |   25 +
>  .../net/ethernet/marvell/prestera/Makefile    |    7 +
>  .../net/ethernet/marvell/prestera/prestera.h  |  208 +++
>  .../marvell/prestera/prestera_devlink.c       |  111 ++
>  .../marvell/prestera/prestera_devlink.h       |   25 +
>  .../ethernet/marvell/prestera/prestera_dsa.c  |  134 ++
>  .../ethernet/marvell/prestera/prestera_dsa.h  |   37 +
>  .../marvell/prestera/prestera_ethtool.c       |  737 ++++++++++
>  .../marvell/prestera/prestera_ethtool.h       |   37 +
>  .../ethernet/marvell/prestera/prestera_hw.c   | 1225 ++++++++++++++++
>  .../ethernet/marvell/prestera/prestera_hw.h   |  180 +++
>  .../ethernet/marvell/prestera/prestera_main.c |  663 +++++++++
>  .../ethernet/marvell/prestera/prestera_pci.c  |  825 +++++++++++
>  .../ethernet/marvell/prestera/prestera_rxtx.c |  860 +++++++++++
>  .../ethernet/marvell/prestera/prestera_rxtx.h |   21 +
>  .../marvell/prestera/prestera_switchdev.c     | 1286 +++++++++++++++++
>  .../marvell/prestera/prestera_switchdev.h     |   16 +
>  20 files changed, 6433 insertions(+)
>  create mode 100644 drivers/net/ethernet/marvell/prestera/Kconfig
>  create mode 100644 drivers/net/ethernet/marvell/prestera/Makefile
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.h
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.c
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.h
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.c
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.h
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.h
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.c
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.h
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_main.c
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_pci.c
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.h
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
>  create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.h
> 
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [net-next 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x)
@ 2020-05-28 15:12 Vadym Kochan
  2020-05-30 14:29 ` Ido Schimmel
  0 siblings, 1 reply; 7+ messages in thread
From: Vadym Kochan @ 2020-05-28 15:12 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
	Andrew Lunn, Oleksandr Mazur, Serhiy Boiko, Serhiy Pshyk,
	Volodymyr Mytnyk, Taras Chornyi, Andrii Savka, netdev,
	linux-kernel
  Cc: Mickey Rachamim, Vadym Kochan

Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8
ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely
wireless SMB deployment.

Prestera Switchdev is a firmware based driver that operates via PCI bus.  The
current implementation supports only boards designed for the Marvell Switchdev
solution and requires special firmware.

This driver implementation includes only L1, basic L2 support, and RX/TX.

The core Prestera switching logic is implemented in prestera_main.c, there is
an intermediate hw layer between core logic and firmware. It is
implemented in prestera_hw.c, the purpose of it is to encapsulate hw
related logic, in future there is a plan to support more devices with
different HW related configurations.

The following Switchdev features are supported:

    - VLAN-aware bridge offloading
    - VLAN-unaware bridge offloading
    - FDB offloading (learning, ageing)
    - Switchport configuration

The firmware image will be uploaded soon to the linux-firmware repository.

PATCH:
    1) Fixed W=1 warnings

    2) Renamed PCI driver name to be more generic "Prestera DX" because
       there will be more devices supported.

    3) Changed firmware image dir path: marvell/ -> mrvl/prestera/
       to be aligned with location in linux-firmware.git (if such
       will be accepted).

RFC v3:
    1) Fix prestera prefix in prestera_rxtx.c

    2) Protect concurrent access from multiple ports on multiple CPU system
       on tx path by spinlock in prestera_rxtx.c

    3) Try to get base mac address from device-tree, otherwise use a random generated one.

    4) Move ethtool interface support into separate prestera_ethtool.c file.

    5) Add basic devlink support and get rid of physical port naming ops.

    6) Add STP support in Switchdev driver.

    7) Removed MODULE_AUTHOR

    8) Renamed prestera.c -> prestera_main.c, and kernel module to
       prestera.ko

RFC v2:
    1) Use "pestera_" prefix in struct's and functions instead of mvsw_pr_

    2) Original series split into additional patches for Switchdev ethtool support.

    3) Use major and minor firmware version numbers in the firmware image filename.

    4) Removed not needed prints.

    5) Use iopoll API for waiting on register's value in prestera_pci.c

    6) Use standart approach for describing PCI ID matching section instead of using
       custom wrappers in prestera_pci.c

    7) Add RX/TX support in prestera_rxtx.c.

    8) Rewritten prestera_switchdev.c with following changes:
       - handle netdev events from prestera.c

       - use struct prestera_bridge for bridge objects, and get rid of
         struct prestera_bridge_device which may confuse.

       - use refcount_t

    9) Get rid of macro usage for sending fw requests in prestera_hw.c

    10) Add base_mac setting as module parameter. base_mac is required for
        generation default port's mac.

Vadym Kochan (6):
  net: marvell: prestera: Add driver for Prestera family ASIC devices
  net: marvell: prestera: Add PCI interface support
  net: marvell: prestera: Add basic devlink support
  net: marvell: prestera: Add ethtool interface support
  net: marvell: prestera: Add Switchdev driver implementation
  dt-bindings: marvell,prestera: Add description for device-tree
    bindings

 .../bindings/net/marvell,prestera.txt         |   34 +
 drivers/net/ethernet/marvell/Kconfig          |    1 +
 drivers/net/ethernet/marvell/Makefile         |    1 +
 drivers/net/ethernet/marvell/prestera/Kconfig |   25 +
 .../net/ethernet/marvell/prestera/Makefile    |    7 +
 .../net/ethernet/marvell/prestera/prestera.h  |  208 +++
 .../marvell/prestera/prestera_devlink.c       |  111 ++
 .../marvell/prestera/prestera_devlink.h       |   25 +
 .../ethernet/marvell/prestera/prestera_dsa.c  |  134 ++
 .../ethernet/marvell/prestera/prestera_dsa.h  |   37 +
 .../marvell/prestera/prestera_ethtool.c       |  737 ++++++++++
 .../marvell/prestera/prestera_ethtool.h       |   37 +
 .../ethernet/marvell/prestera/prestera_hw.c   | 1225 ++++++++++++++++
 .../ethernet/marvell/prestera/prestera_hw.h   |  180 +++
 .../ethernet/marvell/prestera/prestera_main.c |  663 +++++++++
 .../ethernet/marvell/prestera/prestera_pci.c  |  825 +++++++++++
 .../ethernet/marvell/prestera/prestera_rxtx.c |  860 +++++++++++
 .../ethernet/marvell/prestera/prestera_rxtx.h |   21 +
 .../marvell/prestera/prestera_switchdev.c     | 1286 +++++++++++++++++
 .../marvell/prestera/prestera_switchdev.h     |   16 +
 20 files changed, 6433 insertions(+)
 create mode 100644 drivers/net/ethernet/marvell/prestera/Kconfig
 create mode 100644 drivers/net/ethernet/marvell/prestera/Makefile
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_devlink.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_ethtool.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_main.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_pci.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.h
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
 create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-06-20 13:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-28 15:06 [net-next 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x) Vadym Kochan
2020-05-28 15:12 Vadym Kochan
2020-05-30 14:29 ` Ido Schimmel
2020-05-30 14:52   ` Vadym Kochan
2020-05-30 15:54     ` Ido Schimmel
2020-06-01  6:24       ` Jiri Pirko
2020-06-20 13:25         ` Vadym Kochan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.