linux-kernel - Re: [PATCH v1 3/4] can: cast: add driver for CAST CAN controller

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: Mon, 29 Jan 2024 09:26:05 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: William Qiu <william.qiu@...rfivetech.com>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, linux-can@...r.kernel.org, 
	Emil Renner Berthing <kernel@...il.dk>, Rob Herring <robh+dt@...nel.org>, 
	Wolfgang Grandegger <wg@...ndegger.com>, Philipp Zabel <p.zabel@...gutronix.de>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	"David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [PATCH v1 3/4] can: cast: add driver for CAST CAN controller

Hello William Qiu,

thank you for your contribution. I've some quick notes about your
driver.

On 29.01.2024 11:12:38, William Qiu wrote:
> Add driver for CAST CAN Controller. And add compatibility code which
> based on StarFive JH7110 SoC.

Please add yourself or someone else at starfivetech to the Maintainers
file.

Please use BIT() and/or GEN_MASK() to create the _MASK enums. Please use
FIELD_GET(), FIELD_PREP.

Please replace the ccan_ioread8() by a proper 32 bit read and use
FIELD_GET to access any non 32 bit value. Instead of ccan_iowrite8() use
FIELD_PREP and a proper 32 bit write.

The enum ccan_reg_bitchange looks very strange, why do you have OFF and
SET values?

The ccan_reigister_set_bit() and ccan_reigister_off_bit() functions
looks very strange, too. I suggest to use a 32 bit read, set, clear the
bits followed by a 32 bit write. Having set_bit() clear_bit() functions
may lead to more register accesses than needed, if not handled with care.

If you think the driver absolutely needs bit set/clear functions, please
follow the name and signature of the regmap_update_bits(),
regmap_set_bits() and regmap_clear_bits().

Please use can_put_echo_skb(), can_get_echo_skb().

Please implement proper TX-flow control. Stop the TX queue, if you HW
queue is full, start the TX queue once the HW queue has space again.

Consider using the rx_offload helper

You claim you IRQ handler works with shared interrupts, but you return
an error if there are no interrupts by your IP core.

Please enable the clocks during open() and disabled during close()

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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