| 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
| ||
|
Message-ID: <5c240b3b-60c2-45bb-8861-e3a8de28d00f@lunn.ch>
Date: Tue, 24 Oct 2023 02:56:41 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
corbet@....net, steen.hegelund@...rochip.com,
rdunlap@...radead.org, horms@...nel.org, casper.casan@...il.com,
netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
horatiu.vultur@...rochip.com, Woojung.Huh@...rochip.com,
Nicolas.Ferre@...rochip.com, UNGLinuxDriver@...rochip.com,
Thorsten.Kummermehr@...rochip.com
Subject: Re: [PATCH net-next v2 5/9] net: ethernet: oa_tc6: implement
internal PHY initialization
> + /* Minimum supported Chunk Payload Size */
> mincps = FIELD_GET(MINCPS, regval);
> + /* Cut-Through Capability */
> ctc = (regval & CTC) ? true : false;
These comment should be in the patch which added these, not here.
> + /* Direct PHY Register Access Capability */
> + dprac = (regval & DPRAC) ? true : false;
> + /* Indirect PHY Register access Capability */
> + iprac = (regval & IPRAC) ? true : false;
>
> regval = 0;
> oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
> @@ -242,7 +257,7 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
> if (tc6->cps < mincps)
> return -ENODEV;
> } else {
> - tc6->cps = 64;
> + tc6->cps = OA_TC6_MAX_CPS;
This also should of been in an earlier patch.
> }
> if (of_property_present(oa_node, "oa-txcte")) {
> /* Return error if the tx cut through mode is configured
> @@ -266,8 +281,26 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
> regval |= PROTE;
> tc6->prote = true;
> }
> + if (of_property_present(oa_node, "oa-dprac")) {
> + /* Return error if the direct phy register access mode
> + * is configured but it is not supported by MAC-PHY.
> + */
> + if (dprac)
> + tc6->dprac = true;
> + else
> + return -ENODEV;
> + }
This is not in the binding. Why do we even need to be able to
configure it. Direct is faster, so use it is available. If not, use
indirect. And if both dprac and iproc are false, dev_err() and
-ENODEV.
> +static int oa_tc6_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
It would be good to put direct in the name. If somebody implements
indirect, it will make the naming easier.
> +{
> + struct oa_tc6 *tc6 = bus->priv;
> + u32 regval;
> + bool ret;
> +
> + ret = oa_tc6_read_register(tc6, 0xFF00 | (idx & 0xFF), ®val);
> + if (ret)
> + return -ENODEV;
> +
> + return regval;
> +}
> +
> +static int oa_tc6_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
> + u16 val)
> +{
> + struct oa_tc6 *tc6 = bus->priv;
> +
> + return oa_tc6_write_register(tc6, 0xFF00 | (idx & 0xFF), val);
> +}
> +
> +static int oa_tc6_phy_init(struct oa_tc6 *tc6)
> +{
> + int ret;
> +
> + if (tc6->dprac) {
You can avoid the indentation by first checking indirect is the only
choice, and doing a dev_err() followed by return -ENODEV.
> + tc6->mdiobus = mdiobus_alloc();
> + if (!tc6->mdiobus) {
> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
> + return -ENODEV;
> + }
> +
> + tc6->mdiobus->phy_mask = ~(u32)BIT(1);
Does the standard define this ? BIT(1), not BIT(0)?
> /**
> * oa_tc6_init - allocates and intializes oa_tc6 structure.
> * @spi: device with which data will be exchanged.
> - * @prote: control data (register) read/write protection enable/disable.
Something else which should of been in the previous patch. Please look
through this patch and find all the other instances.
> + * @netdev: network device to use.
> *
> * Returns pointer reference to the oa_tc6 structure if all the memory
> * allocation success otherwise NULL.
> */
> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
> {
> struct oa_tc6 *tc6;
>
> @@ -395,15 +521,19 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> if (!tc6)
> return NULL;
>
> + /* Allocate memory for the control tx buffer used for SPI transfer. */
> tc6->ctrl_tx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
> if (!tc6->ctrl_tx_buf)
> return NULL;
>
> + /* Allocate memory for the control rx buffer used for SPI transfer. */
> tc6->ctrl_rx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
> if (!tc6->ctrl_rx_buf)
> return NULL;
>
> tc6->spi = spi;
> + tc6->netdev = netdev;
> + SET_NETDEV_DEV(netdev, &spi->dev);
>
> /* Perform MAC-PHY software reset */
> if (oa_tc6_sw_reset(tc6)) {
> @@ -417,10 +547,27 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> return NULL;
> }
>
> + /* Initialize PHY */
> + if (oa_tc6_phy_init(tc6)) {
> + dev_err(&spi->dev, "PHY initialization failed\n");
> + return NULL;
> + }
> +
> return tc6;
> }
> EXPORT_SYMBOL_GPL(oa_tc6_init);
>
> +/**
> + * oa_tc6_exit - exit function.
> + * @tc6: oa_tc6 struct.
> + *
> + */
> +void oa_tc6_exit(struct oa_tc6 *tc6)
> +{
> + oa_tc6_phy_exit(tc6);
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_exit);
> +
> MODULE_DESCRIPTION("OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface Lib");
> MODULE_AUTHOR("Parthiban Veerasooran <parthiban.veerasooran@...rochip.com>");
> MODULE_LICENSE("GPL");
> diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
> index 378636fd9ca8..36b729c384ac 100644
> --- a/include/linux/oa_tc6.h
> +++ b/include/linux/oa_tc6.h
> @@ -5,54 +5,59 @@
> * Author: Parthiban Veerasooran <parthiban.veerasooran@...rochip.com>
> */
>
> +#include <linux/etherdevice.h>
> #include <linux/spi/spi.h>
>
> /* Control header */
> -#define CTRL_HDR_DNC BIT(31) /* Data-Not-Control */
> -#define CTRL_HDR_HDRB BIT(30) /* Received Header Bad */
> -#define CTRL_HDR_WNR BIT(29) /* Write-Not-Read */
> -#define CTRL_HDR_AID BIT(28) /* Address Increment Disable */
> -#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */
> -#define CTRL_HDR_ADDR GENMASK(23, 8) /* Address */
> -#define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
> -#define CTRL_HDR_P BIT(0) /* Parity Bit */
> +#define CTRL_HDR_DNC BIT(31) /* Data-Not-Control */
> +#define CTRL_HDR_HDRB BIT(30) /* Received Header Bad */
> +#define CTRL_HDR_WNR BIT(29) /* Write-Not-Read */
> +#define CTRL_HDR_AID BIT(28) /* Address Increment Disable */
> +#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */
> +#define CTRL_HDR_ADDR GENMASK(23, 8) /* Address */
> +#define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
> +#define CTRL_HDR_P BIT(0) /* Parity Bit */
Please don't change the whitespace like this.
Andrew
Powered by blists - more mailing lists