Re: [PATCH 04/12] can: kvaser_usb: Mark Mini PCIe 2xHS as supporting error counters

[Anssi Hannula]

On 28.5.2022 10.37, Jimmy Assarsson wrote:
> On 5/16/22 15:47, Anssi Hannula wrote:
>> The 0bfd:0124 Kvaser Mini PCI Express 2xHS (FW 4.18.778) seems to support
>> TX/RX error counters in exactly the same way (via unsolicited cmd 106 on
>> bus errors and via cmd 20 when queried with cmd 19) as 0bfd:0017 Kvaser
>> Memorator Professional HS/HS (FW 2.0.50), but only the latter has
>> KVASER_USB_HAS_TXRX_ERRORS set to enable do_get_berr_counter().
>>
>> Enable error counter retrieval for Kvaser Mini PCI Express 2xHS, too.
>>
>> Fixes: 71873a9b38d1 ("can: kvaser_usb: Add support for more Kvaser Leaf v2 devices")
>> Signed-off-by: Anssi Hannula <anssi.hannula@xxxxxxxxxx>
>>
>> ---
>>
>> I'm not really sure what KVASER_USB_HAS_TXRX_ERRORS means, exactly,
>> w.r.t. device behavior, though, i.e. how does a device without it behave.
> Devices without KVASER_USB_HAS_TXRX_ERRORS, firmware will always report
> zero for the Rx and Tx error counters.
>
> It's possible to query the device for specific capabilities.
> i.e. Kvaser Mini PCI Express 2xHS does also got support for silent mode.
> I want to replace this patch with the one below:

Sounds good!
A couple of minor style comments below. I didn't test the code yet.

>  From fbf1c02e5f7860be9bdafd1c9b4f01c903dd9258 Mon Sep 17 00:00:00 2001
> From: Jimmy Assarsson <extja@xxxxxxxxxx>
> Date: Wed, 25 May 2022 20:21:19 +0200
> Subject: [PATCH 04/13] can: kvaser_usb: kvaser_usb_leaf: Get 
> capabilities from
>   device
>
> Use the CMD_GET_CAPABILITIES_REQ command to query the device for certain
> capabilities. We are only interested in LISTENONLY mode and wither the
> device reports CAN error counters.
>
> And remove hard coded capabilities for all Leaf devices.
>
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB 
> devices")
> Reported-by: Anssi Hannula <anssi.hannula@xxxxxxxxxx>
> Signed-off-by: Jimmy Assarsson <extja@xxxxxxxxxx>
> ---
>   .../net/can/usb/kvaser_usb/kvaser_usb_core.c  |  61 ++------
>   .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 146 +++++++++++++++++-
>   2 files changed, 162 insertions(+), 45 deletions(-)
[...]
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c 
> b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index 09ade66256b2..aee2dae67459 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
[...]
> +static int kvaser_usb_leaf_get_single_capability(struct kvaser_usb *dev,
> +						 u16 cap_cmd_req, u16 *status)
> +{
> +	struct kvaser_usb_dev_card_data *card_data = &dev->card_data;
> +	struct kvaser_cmd *cmd;
> +	u32 value = 0;
> +	u32 mask = 0;
> +	u16 cap_cmd_res;
> +	int err;
> +	int i;
> +
> +	cmd = kcalloc(1, sizeof(struct kvaser_cmd), GFP_KERNEL);

kzalloc can be used here, and prefer sizeof(*ptr) to avoid the risk of
type mismatch:

cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);


[...]
> +static int kvaser_usb_leaf_get_capabilities_leaf(struct kvaser_usb *dev)
> +{
> +	int err;
> +	u16 status;
> +
> +	if (!(dev->card_data.capabilities & KVASER_USB_CAP_EXT_CAP)) {
> +		dev_info(&dev->intf->dev,
> +			 "No extended capability support. Upgrade device firmware.\n");
> +		return 0;
> +	}
> +
> +	err = kvaser_usb_leaf_get_single_capability
> +					(dev,
> +					 KVASER_USB_LEAF_CAP_CMD_LISTEN_MODE,
> +					 &status);

I believe kernel style is to keep the opening parenthesis on the same
line with the function name here.

> +	if (err)
> +		return err;
> +	if (status)
> +		dev_info(&dev->intf->dev,
> +			 "KVASER_USB_LEAF_CAP_CMD_LISTEN_MODE failed %u\n",
> +			 status);
> +
> +	err = kvaser_usb_leaf_get_single_capability
> +					(dev,
> +					 KVASER_USB_LEAF_CAP_CMD_ERR_REPORT,
> +					 &status);


Ditto.

[...]

-- 
Anssi Hannula / Bitwise Oy
+358 503803997