netdev - Re: [PATCH v2 01/12] can: Add LIN bus as CAN abstraction

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: <42012e066d3da1ac89b00b793283d03874cd0776.camel@hexdev.de>
Date: Wed, 08 May 2024 10:37:31 +0200
From: Christoph Fritz <christoph.fritz@...dev.de>
To: Simon Horman <horms@...nel.org>
Cc: Oliver Hartkopp <socketcan@...tkopp.net>, Marc Kleine-Budde
 <mkl@...gutronix.de>, Vincent Mailhol <mailhol.vincent@...adoo.fr>, "David
 S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>,  Paolo Abeni <pabeni@...hat.com>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,  Conor
 Dooley <conor+dt@...nel.org>, Jiri Kosina <jikos@...nel.org>, Benjamin
 Tissoires <bentiss@...nel.org>,  Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, Sebastian
 Reichel <sre@...nel.org>,  Linus Walleij <linus.walleij@...aro.org>,
 Andreas Lauser <andreas.lauser@...cedes-benz.com>, Jonathan Corbet
 <corbet@....net>, Pavel Pisa <pisa@....felk.cvut.cz>,
 linux-can@...r.kernel.org,  netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-input@...r.kernel.org, 
 linux-serial@...r.kernel.org
Subject: Re: [PATCH v2 01/12] can: Add LIN bus as CAN abstraction

On Sat, 2024-05-04 at 13:49 +0100, Simon Horman wrote:
> On Thu, May 02, 2024 at 09:55:23AM +0200, Christoph Fritz wrote:
> > This patch adds a LIN (local interconnect network) bus abstraction on
> > top of CAN.  It is a glue driver adapting CAN on one side while offering
> > LIN abstraction on the other side. So that upcoming LIN device drivers
> > can make use of it.
> > 
> > Tested-by: Andreas Lauser <andreas.lauser@...cedes-benz.com>
> > Signed-off-by: Christoph Fritz <christoph.fritz@...dev.de>
> 
> ...
> 
> > diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c
> 
> ...
> 
> > +struct lin_device *register_lin(struct device *dev,
> > +				const struct lin_device_ops *ldops)
> > +{
> > +	struct net_device *ndev;
> > +	struct lin_device *ldev;
> > +	int ret;
> > +
> > +	if (!ldops || !ldops->ldo_tx || !ldops->update_bitrate  ||
> > +	    !ldops->ldo_open || !ldops->ldo_stop) {
> > +		netdev_err(ndev, "missing mandatory lin_device_ops\n");
> 
> Hi Christoph,

Hi Simon

> The line above uses ndev, but ndev is not initialised
> until a few lines further down.
> 
> Flagged by Smatch.

Despite netdev_err() checks validity of ndev, I agree with Smatch: In
upcoming v4 I'll use dev_err() here instead.

> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	ndev = alloc_candev(sizeof(struct lin_device), 1);
> > +	if (!ndev)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	ldev = netdev_priv(ndev);
> > +
> > +	ldev->ldev_ops = ldops;
> > +	ndev->netdev_ops = &lin_netdev_ops;
> > +	ndev->flags |= IFF_ECHO;
> > +	ndev->mtu = CANFD_MTU;
> > +	ldev->can.bittiming.bitrate = LIN_DEFAULT_BAUDRATE;
> > +	ldev->can.ctrlmode = CAN_CTRLMODE_LIN;
> > +	ldev->can.ctrlmode_supported = 0;
> > +	ldev->can.bitrate_const = lin_bitrate;
> > +	ldev->can.bitrate_const_cnt = ARRAY_SIZE(lin_bitrate);
> > +	ldev->can.do_set_bittiming = lin_set_bittiming;
> > +	ldev->ndev = ndev;
> > +	ldev->dev = dev;
> > +
> > +	SET_NETDEV_DEV(ndev, dev);
> > +
> > +	ret = lin_set_bittiming(ndev);
> > +	if (ret) {
> > +		netdev_err(ndev, "set bittiming failed\n");
> > +		goto exit_candev;
> > +	}
> > +
> > +	ret = register_candev(ndev);
> > +	if (ret)
> > +		goto exit_candev;
> > +
> > +	ldev->lin_ids_kobj = kobject_create_and_add("lin_ids", &ndev->dev.kobj);
> > +	if (!ldev->lin_ids_kobj) {
> > +		netdev_err(ndev, "Failed to create sysfs directory\n");
> > +		ret = -ENOMEM;
> > +		goto exit_unreg;
> > +	}
> > +
> > +	ret = lin_create_sysfs_id_files(ndev);
> > +	if (ret) {
> > +		netdev_err(ndev, "Failed to create sysfs entry: %d\n", ret);
> > +		goto exit_kobj_put;
> > +	}
> > +
> > +	/* Using workqueue as tx over USB/SPI/... may sleep */
> > +	ldev->wq = alloc_workqueue(dev_name(dev), WQ_FREEZABLE | WQ_MEM_RECLAIM,
> > +				   0);
> > +	if (!ldev->wq)
> > +		goto exit_rm_files;
> 
> The goto above will result in: return ERR_PTR(ret)
> But ret is 0 here. Should it be set to a negative error value?
> 
> Also flagged by Smatch.

OK, will get an

ret = -ENOMEM;

> 
> > +
> > +	INIT_WORK(&ldev->tx_work, lin_tx_work_handler);
> > +
> > +	netdev_info(ndev, "LIN initialized.\n");
> > +
> > +	return ldev;
> > +
> > +exit_rm_files:
> > +	lin_remove_sysfs_id_files(ndev);
> > +exit_kobj_put:
> > +	kobject_put(ldev->lin_ids_kobj);
> > +exit_unreg:
> > +	unregister_candev(ndev);
> > +exit_candev:
> > +	free_candev(ndev);
> > +	return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(register_lin);
> 
> ...

Thanks
  -- Christoph