linux-kernel - Re: [RFC PATCH net-next 3/6] net: ethernet: 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: <dd0a6cd5-91e5-4e13-8025-d6c88bdab5a2@lunn.ch>
Date:   Thu, 14 Sep 2023 02:46:03 +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: [RFC PATCH net-next 3/6] net: ethernet: implement OA TC6
 configuration function

> +int oa_tc6_configure(struct oa_tc6 *tc6, u8 cps, bool ctrl_prot, bool tx_cut_thr,
> +		     bool rx_cut_thr)
> +{
> +	u32 regval;
> +	int ret;
> +
> +	/* Read and configure the IMASK0 register for unmasking the interrupts */
> +	ret = oa_tc6_read_register(tc6, OA_TC6_IMASK0, &regval, 1);
> +	if (ret)
> +		return ret;
> +
> +	regval &= TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM;
> +	ret = oa_tc6_write_register(tc6, OA_TC6_IMASK0, &regval, 1);

It is not so obvious what this 1 means. Maybe change to regval[1], and
user ARRAY_SIZE(). What also does not help is the function name,
oa_tc6_write_register(). Singular. So it appears to write one register,
not multiple registers. It might even make sense to make
oa_tc6_write_register() truly access a single register, and add
oa_tc6_write_registers() for multiple registers.

> +/* Unmasking interrupt fields in IMASK0 */
> +#define HDREM		~BIT(5)		/* Header Error Mask */
> +#define LOFEM		~BIT(4)		/* Loss of Framing Error Mask */
> +#define RXBOEM		~BIT(3)		/* Rx Buffer Overflow Error Mask */
> +#define TXBUEM		~BIT(2)		/* Tx Buffer Underflow Error Mask */
> +#define TXBOEM		~BIT(1)		/* Tx Buffer Overflow Error Mask */
> +#define TXPEM		~BIT(0)		/* Tx Protocol Error Mask */

Using ~BIT(X) is very usual. I would not do this, Principle of Least
Surprise.

>  struct oa_tc6 {
> -	struct spi_device *spi;
> -	bool ctrl_prot;
> +	struct completion rst_complete;
>  	struct task_struct *tc6_task;
>  	wait_queue_head_t tc6_wq;
> +	struct spi_device *spi;
> +	bool tx_cut_thr;
> +	bool rx_cut_thr;
> +	bool ctrl_prot;
>  	bool int_flag;
> -	struct completion rst_complete;
> +	u8 cps;
>  };

Please try not to move stuff around. It makes the diff bigger than it
should be.

       Andrew