netdev - Re: [PATCH v4 09/11] net: dsa: microchip: Add Microchip KSZ8863 SMI based driver support

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]

Message-ID: <a1841afd-d5b8-b7f7-a2f1-af94292140fa@gmail.com>
Date:   Mon, 3 Aug 2020 11:12:30 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Michael Grzeschik <m.grzeschik@...gutronix.de>, andrew@...n.ch
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kernel@...gutronix.de
Subject: Re: [PATCH v4 09/11] net: dsa: microchip: Add Microchip KSZ8863 SMI
 based driver support



On 8/2/2020 10:44 PM, Michael Grzeschik wrote:
> Add KSZ88X3 driver support. We add support for the KXZ88X3 three port
> switches using the Microchip SMI Interface. They are supported using the
> MDIO-Bitbang Interface.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
> 

[snip

> +
> +config NET_DSA_MICROCHIP_KSZ8863_SMI
> +	tristate "KSZ series SMI connected switch driver"
> +	depends on NET_DSA_MICROCHIP_KSZ8795
> +	select MDIO_BITBANG
> +	default y

As Randy already identified please remove this.

> +	help
> +	  Select to enable support for registering switches configured through
> +	  Microchip SMI. It Supports the KSZ8863 and KSZ8873 Switch.
> diff --git a/drivers/net/dsa/microchip/Makefile b/drivers/net/dsa/microchip/Makefile
> index 929caa81e782ed2..2a03b21a3386f5d 100644
> --- a/drivers/net/dsa/microchip/Makefile
> +++ b/drivers/net/dsa/microchip/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C)	+= ksz9477_i2c.o
>  obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_SPI)	+= ksz9477_spi.o
>  obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8795)		+= ksz8795.o
>  obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8795_SPI)	+= ksz8795_spi.o
> +obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8863_SMI)	+= ksz8863_smi.o
> diff --git a/drivers/net/dsa/microchip/ksz8863_smi.c b/drivers/net/dsa/microchip/ksz8863_smi.c
> new file mode 100644
> index 000000000000000..fd493441d725284
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz8863_smi.c
> @@ -0,0 +1,204 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip KSZ8863 series register access through SMI
> + *
> + * Copyright (C) 2019 Pengutronix, Michael Grzeschik <kernel@...gutronix.de>
> + */
> +
> +#include "ksz8.h"
> +#include "ksz_common.h"
> +
> +/* Serial Management Interface (SMI) uses the following frame format:
> + *
> + *       preamble|start|Read/Write|  PHY   |  REG  |TA|   Data bits      | Idle
> + *               |frame| OP code  |address |address|  |                  |
> + * read | 32x1´s | 01  |    00    | 1xRRR  | RRRRR |Z0| 00000000DDDDDDDD |  Z
> + * write| 32x1´s | 01  |    00    | 0xRRR  | RRRRR |10| xxxxxxxxDDDDDDDD |  Z
> + *
> + */
> +
> +static int ksz8863_mdio_read(void *ctx, const void *reg_buf, size_t reg_len,
> +			     void *val_buf, size_t val_len)
> +{
> +	struct ksz_device *dev = (struct ksz_device *)ctx;

There is no need to cast a void pointer, can you also make it a const
void *ctx?

> +	struct ksz8 *ksz8 = dev->priv;
> +	struct mdio_device *mdev = ksz8->priv;
> +	u8 reg = *(u8 *)reg_buf;
> +	u8 *val = val_buf;
> +	int ret = 0;
> +	int i;
> +
> +	mutex_lock_nested(&mdev->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +	for (i = 0; i < val_len; i++) {
> +		int tmp = reg + i;

Humm, how does this work for reg_len >1 with reg being a scalar? Why not
just use reg[i]?

> +
> +		ret = __mdiobus_read(mdev->bus, ((tmp & 0xE0) >> 5) |
> +				     BIT(4), tmp);

Can we provide defines instead of these numbers?

> +		if (ret < 0)
> +			goto out;
> +
> +		val[i] = ret;
> +	}
> +	ret = 0;
> +
> + out:
> +	mutex_unlock(&mdev->bus->mdio_lock);
> +
> +	return ret;
> +}
> +
> +static int ksz8863_mdio_write(void *ctx, const void *data, size_t count)
> +{
> +	struct ksz_device *dev = (struct ksz_device *)ctx;

Likewise, no need to cast here.

> +	struct ksz8 *ksz8 = dev->priv;
> +	struct mdio_device *mdev = ksz8->priv;
> +	u8 *val = (u8 *)(data + 4);
> +	u32 reg = *(u32 *)data;

Humm, are you positive this works for all endian configurations?
-- 
Florian