From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Christoph Fritz <christoph.fritz@hexdev.de>
Cc: Jiri Slaby <jirislaby@kernel.org>,
Simon Horman <horms@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Marc Kleine-Budde <mkl@pengutronix.de>,
Oliver Hartkopp <socketcan@hartkopp.net>,
Vincent Mailhol <mailhol.vincent@wanadoo.fr>,
"David S . Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
Paolo Abeni <pabeni@redhat.com>, Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <bentiss@kernel.org>,
Sebastian Reichel <sre@kernel.org>,
Linus Walleij <linus.walleij@linaro.org>,
Andreas Lauser <andreas.lauser@mercedes-benz.com>,
Jonathan Corbet <corbet@lwn.net>,
Pavel Pisa <pisa@cmp.felk.cvut.cz>,
linux-can@vger.kernel.org, Netdev <netdev@vger.kernel.org>,
devicetree@vger.kernel.org, linux-input@vger.kernel.org,
linux-serial <linux-serial@vger.kernel.org>
Subject: Re: [PATCH v4 02/11] HID: hexLIN: Add support for USB LIN adapter
Date: Mon, 13 May 2024 15:27:56 +0300 (EEST) [thread overview]
Message-ID: <d91451f2-5636-2526-391d-f575a7feb17c@linux.intel.com> (raw)
In-Reply-To: <8738628a5c1b87c6521fdd8d05a3b36e5c32b48a.camel@hexdev.de>
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.
next prev parent reply other threads:[~2024-05-13 12:28 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-09 17:17 [PATCH v4 00/11] LIN Bus support for Linux Christoph Fritz
2024-05-09 17:17 ` [PATCH v4 01/11] can: Add LIN bus as CAN abstraction Christoph Fritz
2024-05-10 13:23 ` Ilpo Järvinen
2024-05-13 9:52 ` Simon Horman
2024-05-09 17:17 ` [PATCH v4 02/11] HID: hexLIN: Add support for USB LIN adapter Christoph Fritz
2024-05-10 13:46 ` Ilpo Järvinen
2024-05-11 8:14 ` Christoph Fritz
2024-05-13 12:27 ` Ilpo Järvinen [this message]
2024-05-13 5:26 ` Jiri Slaby
2024-05-09 17:17 ` [PATCH v4 03/11] treewide, serdev: add flags argument to receive_buf() Christoph Fritz
2024-05-10 14:11 ` Ilpo Järvinen
2024-05-09 17:17 ` [PATCH v4 04/11] tty: serdev: Add method to enable break flags Christoph Fritz
2024-05-10 14:21 ` Ilpo Järvinen
2024-05-12 13:08 ` Christoph Fritz
2024-05-13 12:30 ` Ilpo Järvinen
2024-05-09 17:17 ` [PATCH v4 05/11] dt-bindings: vendor-prefixes: Add hexDEV Christoph Fritz
2024-05-09 17:17 ` [PATCH v4 06/11] dt-bindings: net/can: Add serial LIN adapter hexLINSER Christoph Fritz
2024-05-09 18:09 ` Conor Dooley
2024-05-09 17:17 ` [PATCH v4 07/11] can: Add support for hexDEV " Christoph Fritz
2024-05-10 14:32 ` Ilpo Järvinen
2024-05-09 17:17 ` [PATCH v4 08/11] can: bcm: Add LIN answer offloading for responder mode Christoph Fritz
2024-05-09 17:17 ` [PATCH v4 09/11] can: lin: Handle rx offload config frames Christoph Fritz
2024-05-10 14:36 ` Ilpo Järvinen
2024-05-09 17:17 ` [PATCH v4 10/11] can: lin: Support setting LIN mode Christoph Fritz
2024-05-10 14:39 ` Ilpo Järvinen
2024-05-09 17:17 ` [PATCH v4 11/11] HID: hexLIN: Implement ability to update lin mode Christoph Fritz
2024-05-18 18:29 ` [PATCH v4 00/11] LIN Bus support for Linux Christoph Fritz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d91451f2-5636-2526-391d-f575a7feb17c@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=andreas.lauser@mercedes-benz.com \
--cc=bentiss@kernel.org \
--cc=christoph.fritz@hexdev.de \
--cc=conor+dt@kernel.org \
--cc=corbet@lwn.net \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=edumazet@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=horms@kernel.org \
--cc=jikos@kernel.org \
--cc=jirislaby@kernel.org \
--cc=krzk+dt@kernel.org \
--cc=kuba@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=mailhol.vincent@wanadoo.fr \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=pisa@cmp.felk.cvut.cz \
--cc=robh@kernel.org \
--cc=socketcan@hartkopp.net \
--cc=sre@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).