netdev - Re: [RFC 2/3] ksz: Add Microchip KSZ8873 SMI-DSA driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 10 May 2019 09:37:55 +0200
From:   Michael Grzeschik <mgr@...gutronix.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Tristram.Ha@...rochip.com, kernel@...gutronix.de,
        UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org
Subject: Re: [RFC 2/3] ksz: Add Microchip KSZ8873 SMI-DSA driver

On Thu, May 09, 2019 at 04:48:44PM +0200, Andrew Lunn wrote:
> On Wed, May 08, 2019 at 11:13:29PM +0200, Michael Grzeschik wrote:
> > Cc: Tristram.Ha@...rochip.com
> > Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
> > ---
> >  drivers/net/dsa/microchip/Kconfig       |   16 +
> >  drivers/net/dsa/microchip/Makefile      |    2 +
> >  drivers/net/dsa/microchip/ksz8863.c     | 1026 +++++++++++++++++++++++
> >  drivers/net/dsa/microchip/ksz8863_reg.h |  605 +++++++++++++
> >  drivers/net/dsa/microchip/ksz8863_smi.c |  105 +++
> >  drivers/net/dsa/microchip/ksz_priv.h    |    3 +
> >  include/net/dsa.h                       |    2 +
> >  net/dsa/Kconfig                         |    7 +
> >  net/dsa/tag_ksz.c                       |   45 +
> >  9 files changed, 1811 insertions(+)
> >  create mode 100644 drivers/net/dsa/microchip/ksz8863.c
> >  create mode 100644 drivers/net/dsa/microchip/ksz8863_reg.h
> >  create mode 100644 drivers/net/dsa/microchip/ksz8863_smi.c
> > 
> > diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
> > index bea29fde9f3d1..a6fa6ae972951 100644
> > --- a/drivers/net/dsa/microchip/Kconfig
> > +++ b/drivers/net/dsa/microchip/Kconfig
> > @@ -14,3 +14,19 @@ config NET_DSA_MICROCHIP_KSZ9477_SPI
> >  	depends on NET_DSA_MICROCHIP_KSZ9477 && SPI
> >  	help
> >  	  Select to enable support for registering switches configured through SPI.
> > +
> > +menuconfig NET_DSA_MICROCHIP_KSZ8863
> > +	tristate "Microchip KSZ8863 series switch support"
> > +	depends on NET_DSA
> > +	select NET_DSA_TAG_KSZ8863
> > +	select NET_DSA_MICROCHIP_KSZ_COMMON
> > +	help
> > +	  This driver adds support for Microchip KSZ8863 switch chips.
> > +
> > +config NET_DSA_MICROCHIP_KSZ8863_SMI
> 
> > +	tristate "KSZ series SMI connected switch driver"
> > +	depends on NET_DSA_MICROCHIP_KSZ8863
> > +	default y
> > +	help
> > +	  Select to enable support for registering switches configured through SMI.
> 
> SMI is a synonym for MDIO. So we should make it clear, this is a
> proprietary version. "... through Microchip SMI".
> 
> You might also want to either depend on or select mdio-bitbang, since
> that is the other driver which supports Microchip SMI.

Right, I will add the comment and include the depend to mdio-bitbang.

> > +static int ksz_spi_read(struct ksz_device *dev, u32 reg, u8 *data,
> > +			unsigned int len)
> > +{
> > +	int i = 0;
> > +
> > +	for (i = 0; i < len; i++)
> > +		data[i] = (u8)mdiobus_read(dev->bus, 0,
> > +					    (reg + i) | MII_ADDR_SMI0);
> 
> mdiobus_read() and mdiobus_write() can return an error, which is why
> is returns an int. Please check for the error and return it.

Thanks for pointing this out. I will fix it!

> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index 6aaaadd6a413c..57fbf3e722362 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -44,6 +44,7 @@ struct phylink_link_state;
> >  #define DSA_TAG_PROTO_TRAILER_VALUE		11
> >  #define DSA_TAG_PROTO_8021Q_VALUE		12
> >  #define DSA_TAG_PROTO_SJA1105_VALUE		13
> > +#define DSA_TAG_PROTO_KSZ8863_VALUE		14
> >  
> >  enum dsa_tag_protocol {
> >  	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
> > @@ -60,6 +61,7 @@ enum dsa_tag_protocol {
> >  	DSA_TAG_PROTO_TRAILER		= DSA_TAG_PROTO_TRAILER_VALUE,
> >  	DSA_TAG_PROTO_8021Q		= DSA_TAG_PROTO_8021Q_VALUE,
> >  	DSA_TAG_PROTO_SJA1105		= DSA_TAG_PROTO_SJA1105_VALUE,
> > +	DSA_TAG_PROTO_KSZ8863		= DSA_TAG_PROTO_KSZ8863_VALUE,
> >  };
> 
> Please put all the tag driver changes into a separate patch.

Right, I will ad that to another patch.

> > +static struct sk_buff *ksz8863_rcv(struct sk_buff *skb, struct net_device *dev,
> > +				   struct packet_type *pt)
> > +{
> > +	/* Tag decoding */
> > +	u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
> > +	unsigned int port = tag[0] & 1;
> 
> Does this device only have 2 ports, 0 and 1?
> 

Yes this is a very basic "switch" only managing two physical ports.

Refer Page 28 Chapter 3.7 "Tail Tagging Mode"
in the Reference Manual of the switch:

http://ww1.microchip.com/downloads/en/DeviceDoc/00002335B.pdf

> > +	unsigned int len = KSZ_EGRESS_TAG_LEN;
> > +
> > +	return ksz_common_rcv(skb, dev, port, len);
> > +}
> 
>   Andrew
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)