Re: [PATCH v7 07/15] can: grcan: add FD capability detection and nominal bit-timing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing assignment of `priv->can.do_set_bittiming` breaks hardware bit-timing configuration.
- [Medium] Missing NULL check on `device_get_match_data()` leads to potential kernel panic.
--

commit 37c33b36466e8784c1911d95ae987b6dfff15154
Author: Arun Muthusamy <arun.muthusamy@gaisler.com>

can: grcan: add FD capability detection and nominal bit-timing

This commit adds CAN FD capability detection and structures for setting
nominal bit-timing for standard CAN FD. It also updates the device tree
match table and Kconfig for GRCANFD support.

> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 8ba21d94e87df..c9175a6a013fc 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c

[ ... ]

> @@ -1567,14 +1646,14 @@ static int grcan_setup_netdev(struct platform_device *ofdev,
>  	priv->dev = dev;
>  	priv->ofdev_dev = &ofdev->dev;
>  	priv->regs = base;
> -	priv->can.bittiming_const = &grcan_bittiming_const;
> -	priv->can.do_set_bittiming = grcan_set_bittiming;
> +	priv->can.bittiming_const = hwcap->bt_const;

Will the hardware bit-timing still be configured correctly without this
callback?

It appears the assignment to priv->can.do_set_bittiming was removed but not
replaced with hwcap->set_bittiming. If this is left as NULL, the CAN
framework will skip programming the hardware's bit-timing registers and
return 0. This could cause the hardware to attempt transmissions using
uninitialized registers, potentially leading to continuous error frames on
the bus.

[ ... ]

> @@ -1619,22 +1698,29 @@ static int grcan_probe(struct platform_device *ofdev)
>  {
>  	struct device_node *np = ofdev->dev.of_node;
>  	struct device_node *sysid_parent;
> +	const struct grcan_hwcap *hwcap;
>  	struct clk *clk;
>  	u32 sysid, ambafreq;
>  	int irq, err;
>  	void __iomem *base;
>  	bool txbug = true;
>  
> +	hwcap = device_get_match_data(&ofdev->dev);
> +
>  	/* Compare GRLIB version number with the first that does not
>  	 * have the tx bug (see start_xmit)
>  	 */
> -	sysid_parent = of_find_node_by_path("/ambapp0");
> -	if (sysid_parent) {
> -		err = of_property_read_u32(sysid_parent, "systemid", &sysid);
> -		if (!err && ((sysid & GRLIB_VERSION_MASK) >=
> -			     GRCAN_TXBUG_SAFE_GRLIB_VERSION))
> -			txbug = false;
> -		of_node_put(sysid_parent);
> +	if (!hwcap->txbug_possible) {

Could this cause a regression if device_get_match_data() returns NULL?

If the driver is instantiated via a platform device match (matching the
driver string) rather than through Open Firmware, device_get_match_data()
will return NULL. Dereferencing hwcap immediately could result in a NULL
pointer dereference. Is a fallback or check needed here before accessing
hwcap->txbug_possible?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508070121.6918-1-arun.muthusamy@gaisler.com?part=7