netdev - Re: [PATCH net-next v2 3/9] net: ethernet: oa_tc6: implement OA TC6 configuration function

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: <423e0b42-a75e-4104-b445-7d9ff0991acf@lunn.ch>
Date: Tue, 24 Oct 2023 00:58:51 +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 3/9] net: ethernet: oa_tc6: implement OA TC6
 configuration function

> +	/* Read and configure the IMASK0 register for unmasking the interrupts */
> +	ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, false, false);
> +	if (ret)
> +		return ret;

Can you use oa_tc6_read_register() here? I guess the question is, what
does tc6->protect default to until it is set later in this function?
So long as it defaults to false, i guess you can use the register
read/write functions, which are a lot more readable than this generic
oa_tc6_perform_ctrl().

> +
> +	regval &= ~(TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM);
> +
> +	ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, true, false);
> +	if (ret)
> +		return ret;
> +
> +	/* Read STDCAP register to get the MAC-PHY standard capabilities */
> +	ret = oa_tc6_perform_ctrl(tc6, STDCAP, &regval, 1, false, false);
> +	if (ret)
> +		return ret;
> +
> +	mincps = FIELD_GET(MINCPS, regval);
> +	ctc = (regval & CTC) ? true : false;
> +
> +	regval = 0;
> +	oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
> +	if (oa_node) {
> +		/* Read OA parameters from DT */
> +		if (of_property_present(oa_node, "oa-cps")) {
> +			ret = of_property_read_u32(oa_node, "oa-cps", &tc6->cps);

If of_property_read_u32() does not find the property, it is documented
to not touch tc6->cps. So you can set tc6->cps to the default 64,
before the big if, and skip the of_property_present(). You can then
probably remove the else at the end as well.

> +			if (ret < 0)
> +				return ret;
> +			/* Return error if the configured cps is less than the
> +			 * minimum cps supported by the MAC-PHY.
> +			 */
> +			if (tc6->cps < mincps)
> +				return -ENODEV;

A dev_err() would be nice here to indicate why.

> +		} else {
> +			tc6->cps = 64;
> +		}
> +		if (of_property_present(oa_node, "oa-txcte")) {
> +			/* Return error if the tx cut through mode is configured
> +			 * but it is not supported by MAC-PHY.
> +			 */
> +			if (ctc)
> +				regval |= TXCTE;
> +			else
> +				return -ENODEV;

and a dev_err() here as well.

> +		}
> +		if (of_property_present(oa_node, "oa-rxcte")) {
> +			/* Return error if the rx cut through mode is configured
> +			 * but it is not supported by MAC-PHY.
> +			 */
> +			if (ctc)
> +				regval |= RXCTE;
> +			else
> +				return -ENODEV;
> +		}

and another dev_err(). Without these prints, you probably need to
modify the code to figure out why the probe failed.

> +		if (of_property_present(oa_node, "oa-prote")) {
> +			regval |= PROTE;
> +			tc6->prote = true;
> +		}
> +	} else {
> +		tc6->cps = 64;
> +	}
> +
> +	regval |= FIELD_PREP(CPS, ilog2(tc6->cps) / ilog2(2)) | SYNC;
> +
> +	return oa_tc6_perform_ctrl(tc6, CONFIG0, &regval, 1, true, false);
> +}
> +
>  static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
>  {
>  	u32 regval;
> @@ -310,7 +387,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
>   * 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, bool prote)
> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)

Was there a reason to have prote initially, and then remove it here?

    Andrew