Message-ID: <4186d48a-ea7e-39c1-d1fa-1db3f6627a3a@datenfreihafen.org>
Date:   Wed, 5 Jan 2022 21:27:43 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.1.0
Subject: Re: [PATCH -next] ieee802154: atusb: move to new USB API
Content-Language: en-US
To:     Pavel Skripkin <paskripkin@gmail.com>, alex.aring@gmail.com,
        davem@davemloft.net, kuba@kernel.org, linux-wpan@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220105144947.12540-1-paskripkin@gmail.com>
From:   Stefan Schmidt <stefan@datenfreihafen.org>
In-Reply-To: <20220105144947.12540-1-paskripkin@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


Hello.

On 05.01.22 15:49, Pavel Skripkin wrote:
> Old USB API is prone to uninit value bugs if error handling is not
> correct. Let's move atusb to use new USB API to
> 
> 	1) Make code more simple, since new API does not require memory
> 	   to be allocates via kmalloc()
> 
> 	2) Defend driver from usb-related uninit value bugs.
> 
> 	3) Make code more modern and simple
> 
> This patch removes atusb usb wrappers as Greg suggested [0], this will make
> code more obvious and easier to understand over time, and replaces old
> API calls with new ones.
> 
> Also this patch adds and updates usb related error handling to prevent
> possible uninit value bugs in future
> 
> Link: https://lore.kernel.org/all/YdL0GPxy4TdGDzOO@kroah.com/ [0]
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
> 
> Only build tested.

Gave it a first quick run on real hardware here. Besides one small bug 
(see below) it looked good.

Will give it a bit more testing over the next days.


> @@ -881,14 +819,27 @@ static int atusb_get_and_conf_chip(struct atusb *atusb)
>   	u8 man_id_0, man_id_1, part_num, version_num;
>   	const char *chip;
>   	struct ieee802154_hw *hw = atusb->hw;
> +	int ret;
>   
> -	man_id_0 = atusb_read_reg(atusb, RG_MAN_ID_0);
> -	man_id_1 = atusb_read_reg(atusb, RG_MAN_ID_1);
> -	part_num = atusb_read_reg(atusb, RG_PART_NUM);
> -	version_num = atusb_read_reg(atusb, RG_VERSION_NUM);
> +	ret = usb_control_msg_recv(usb_dev, 0, ATUSB_REG_READ, ATUSB_REQ_FROM_DEV,
> +				   0, RG_MAN_ID_0, &man_id_0, 1, 1000, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
>   
> -	if (atusb->err)
> -		return atusb->err;
> +	ret = usb_control_msg_recv(usb_dev, 0, ATUSB_REG_READ, ATUSB_REQ_FROM_DEV,
> +				   0, RG_MAN_ID_1, &man_id_1, 1, 1000, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_control_msg_recv(usb_dev, 0, ATUSB_REG_READ, ATUSB_REQ_FROM_DEV,
> +				   0, RG_PART_NUM, &atusb, 1, 1000, GFP_KERNEL);

This needs to be written to &part_num and not &atusb.

Pretty nice for a first blind try without hardware. Thanks.

Will let you know if I find anything else from testing.

regards
Stefan Schmidt