Date: Mon, 13 May 2024 15:27:56 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Christoph Fritz <christoph.fritz@...dev.de>
cc: Jiri Slaby <jirislaby@...nel.org>, Simon Horman <horms@...nel.org>, 
    Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
    Marc Kleine-Budde <mkl@...gutronix.de>, 
    Oliver Hartkopp <socketcan@...tkopp.net>, 
    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>, 
    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 <netdev@...r.kernel.org>, 
    devicetree@...r.kernel.org, linux-input@...r.kernel.org, 
    linux-serial <linux-serial@...r.kernel.org>
Subject: Re: [PATCH v4 02/11] HID: hexLIN: Add support for USB LIN adapter

On Sat, 11 May 2024, Christoph Fritz wrote:

> ...
> > > diff --git a/drivers/hid/hid-hexdev-hexlin.c b/drivers/hid/hid-hexdev-hexlin.c
> > > new file mode 100644
> > > index 0000000000000..a9ed080b3e33e
> > > --- /dev/null
> > > +++ b/drivers/hid/hid-hexdev-hexlin.c
> > > 
> ...
> > 
> > > +}
> > > +
> > > +#define HEXLIN_GET_CMD(name, enum_cmd)					\
> > > +	static int hexlin_##name(struct hexlin_priv_data *p)		\
> > > +	{								\
> > > +		u8 *req;						\
> > > +		int ret;						\
> > > +									\
> > > +		req = kmalloc(sizeof(*req), GFP_KERNEL)	;		\
> > 
> > Extra space.
> > 
> > Use:
> > 
> > u8 *req __free(kfree) = kmalloc(sizeof(*req), GFP_KERNEL);
> > 
> > > +		if (!req)						\
> > > +			return -ENOMEM;					\
> > > +									\
> > > +		*req = enum_cmd;					\
> > > +									\
> > > +		ret = hexlin_tx_req_status(p, req, sizeof(*req));	\
> > > +		if (ret)						\
> > > +			hid_err(p->hid_dev, "%s failed, error: %d\n",	\
> > > +				#name, ret);				\
> > > +									\
> > > +		kfree(req);						\
> > 
> > Not needed after using __free().
> > 
> > > +		return ret;						\
> > > +	}
> > > +
> > > +HEXLIN_GET_CMD(get_version, HEXLIN_GET_VERSION)
> > > +HEXLIN_GET_CMD(reset_dev, HEXLIN_RESET)
> > > +HEXLIN_GET_CMD(get_baudrate, HEXLIN_GET_BAUDRATE)
> > 
> > Could you convert the function in the macro into a helper function which 
> > is just called by a simple function with the relevant parameters for 
> > these 3 cases?
> 
> The device actually has a lot more features that I'm using in my debug
> version and which might end up here in the future. So I would like to
> keep it. Any objections?

I don't follow, HEXLIN_GET_CMD() will always produce a copy of that same 
function even if you use it 1000 times?? (Except for the enum and string 
which can easily be passed as arguments to a common function).

You can still keep HEXLIN_GET_CMD() which just calls that common function
if you want to avoid giving those two parameters directly.

> > > +static int hexlin_set_baudrate(struct hexlin_priv_data *priv, u16 baudrate)
> > > +{
> > > +	struct hexlin_baudrate_req *req;
> > > +	int ret;
> > > +
> > > +	if (baudrate < LIN_MIN_BAUDRATE || baudrate > LIN_MAX_BAUDRATE)
> > > +		return -EINVAL;
> > > +
> > > +	req = kmalloc(sizeof(*req), GFP_KERNEL);
> > 
> > Hmm... Why do you alloc this small structure (3 bytes?) with kmalloc() and 
> > not just have it in stack as a local variable?
> 
> This buffer must be suitable for DMA (see docu for struct urb).
> 
> So with a stack variable we would need to use kmemdup() before the
> actual sending call, but that's what you did not like since v3 so I
> changed it to this which now you also don't like.
>
> Let's dial it back to the original kmemdup() usage, ok?

I asked a question. Since you have a good reason for alloc, just keep the 
alloc then.

Now I notice it's kmalloc(), why not kzalloc()?

> > > +static int hexlin_queue_frames_insert(struct hexlin_priv_data *priv,
> > > +				      const struct hexlin_frame *hxf)
> > > +{
> > > +	struct hid_device *hdev = priv->hid_dev;
> > > +	struct lin_frame lf;
> > > +
> > > +	lf.len = hxf->len;
> > > +	lf.lin_id = hxf->lin_id;
> > > +	memcpy(lf.data, hxf->data, LIN_MAX_DLEN);
> > > +	lf.checksum = hxf->checksum;
> > > +	lf.checksum_mode = hxf->checksum_mode;
> > > +
> > > +	hid_dbg(hdev, "id:%02x, len:%u, data:%*ph, chk:%02x (%s), flg:%08x\n",
> > > +		lf.lin_id, lf.len, lf.len, lf.data, lf.checksum,
> > > +		lf.checksum_mode ? "enhanced" : "classic", hxf->flags);
> > > +
> > > +	lin_rx(priv->ldev, &lf);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int hexlin_raw_event(struct hid_device *hdev,
> > > +			    struct hid_report *report, u8 *data, int sz)
> > > +{
> > > +	struct hexlin_priv_data *priv;
> > > +	struct hexlin_baudrate_req *br;
> > > +	struct hexlin_responder_answer_req *rar;
> > > +	struct hexlin_unconditional_req *hfr;
> > > +	struct hexlin_val8_req *vr;
> > > +
> > > +	if (sz < 1 || sz > HEXLIN_PKGLEN_MAX)
> > > +		return -EREMOTEIO;
> > > +
> > > +	priv = hid_get_drvdata(hdev);
> > > +
> > > +	hid_dbg(hdev, "%s, size:%i, data[0]: 0x%02x\n", __func__, sz, data[0]);
> > > +
> > > +	priv->is_error = false;
> > > +
> > > +	switch (data[0]) {
> > > +	case HEXLIN_SUCCESS:
> > > +		if (sz != HEXLIN_LEN_RETCODE)
> > > +			return -EREMOTEIO;
> > > +		hid_dbg(hdev, "HEXLIN_SUCCESS: 0x%02x\n", data[0]);
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	case HEXLIN_FAIL:
> > > +		if (sz != HEXLIN_LEN_RETCODE)
> > > +			return -EREMOTEIO;
> > > +		hid_err(hdev, "HEXLIN_FAIL: 0x%02x\n", data[0]);
> > > +		priv->is_error = true;
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	case HEXLIN_GET_VERSION:
> > > +		if (sz != sizeof(*vr))
> > > +			return -EREMOTEIO;
> > > +		vr = (struct hexlin_val8_req *) data;
> > > +		priv->fw_version = vr->v;
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	case HEXLIN_GET_RESPONDER_ANSWER_ID:
> > > +		if (sz != sizeof(*rar))
> > > +			return -EREMOTEIO;
> > > +		rar = (struct hexlin_responder_answer_req *) data;
> > > +		memcpy(&priv->answ, &rar->answ, sizeof(priv->answ));
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	case HEXLIN_GET_BAUDRATE:
> > > +		if (sz != sizeof(*br))
> > > +			return -EREMOTEIO;
> > > +		br = (struct hexlin_baudrate_req *) data;
> > > +		le16_to_cpus(br->baudrate);
> > > +		priv->baudrate = br->baudrate;
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	/* following cases not initiated by us, so no complete() */
> > > +	case HEXLIN_FRAME:
> > > +		if (sz != sizeof(*hfr)) {
> > > +			hid_err_once(hdev, "frame size mismatch: %i\n", sz);
> > > +			return -EREMOTEIO;
> > > +		}
> > > +		hfr = (struct hexlin_unconditional_req *) data;
> > > +		le32_to_cpus(hfr->frm.flags);
> > 
> > I'm bit worried about this from endianness perspective. Perhaps there's 
> > some struct reusing that shouldn't be happening because the same field 
> > cannot be __le32 and u32 at the same time.
> 
> Can you propose a solution?

Just do a le32_to_cpu(hfr->frm.flags) in the debug print's arguments?

-- 
 i.

Powered by blists - more mailing lists