Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 19 May 2022 07:47:43 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Pavel Skripkin <paskripkin@gmail.com>
Cc:     gregkh@linuxfoundation.org, Larry.Finger@lwfinger.net,
        phil@philpotter.co.uk, straube.linux@gmail.com,
        fmdefrancesco@gmail.com, linux-kernel@vger.kernel.org,
        linux-staging@lists.linux.dev
Subject: Re: [PATCH 2/4] staging: r8188eu: add error handling of rtw_read16
Message-ID: <20220519044743.GT4009@kadam>
References: <cover.1652911343.git.paskripkin@gmail.com>
 <fae229ad24be682407c85fb25ea1ce4d79d83fcd.1652911343.git.paskripkin@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fae229ad24be682407c85fb25ea1ce4d79d83fcd.1652911343.git.paskripkin@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?PuJf5rimg5s8S959C9dmeasVrlUCJQPNFI/WwwQlO7KuRdYsySMrZnFgCnI6?=
 =?us-ascii?Q?z/5ikm6x0OoQXZiV0az1sc639duXofF7Te9zQOcHuqcT78+qIuiZymw0QXmb?=
 =?us-ascii?Q?iMk4RqlPLRb1NQzOrZ9mdN6kLPEW60yJlyjSi007T3NKQaL5/b33mAyhpS2N?=
 =?us-ascii?Q?yTn8sY8El0kHC7VqZRl/AxrF/IvAQJeg6LQd7iuRS/MNnUf2f4VpzJ2oYr/G?=
 =?us-ascii?Q?UqJfiwkfxA7WBvOPSJf56j7QQYz2/eTpQWBAgg3TL9MoS8DSoAJ84T6iACO7?=
 =?us-ascii?Q?nUYwwqaftJ5b0xYullfUXFyiD6Z2piSHov+vV5DrJqG1uGofIzXuSuM6QSAe?=
 =?us-ascii?Q?Ueo5T9Nzp12T3eygT++VNMmoO/Vm2tIEs/v1H1WkE7dUXgTGEXfIxg6z1dZR?=
 =?us-ascii?Q?p5sjLeWKiWFdp20g6I0iWZK+WmWM5DtJCq8hQCWDRKbjFWXPwvHchPMPEgil?=
 =?us-ascii?Q?OKXZPLPY+jpOOcKgJ2G/CP+qsR3zIaHBGXWETck+jFnGCDfcP9wzgp9kb6u8?=
 =?us-ascii?Q?3sUOmYjHRowYUtwvIQGEq0QgyzqQ+4T789o2NfraEqJm495cW625bObm6bHf?=
 =?us-ascii?Q?J6+9GVa4yYTd/GzTd4OmrKzDFbpgcWszWOUoQ3dNx/D+hHt1VkIApQzUa9FF?=
 =?us-ascii?Q?a7ocfsir86+oD9yBznvclhiHu4IAqzw7+Gum2EX/K/5za69zVyAveW2Q5q0b?=
 =?us-ascii?Q?dZlgPWWzPf4iHNLIbZ3jO8/KGLgBAmtc47yUZ6W4jKtVJQy0Kz/eAVi2DGf4?=
 =?us-ascii?Q?Dv9l0WtDInqrgpmLMorZehQSQ1y2kmwR+V5vXpAmcOf+4O/+3UFwbg8FdWcU?=
 =?us-ascii?Q?mbtanbcW8AQ3j6Ww7mL3s95gXAS+uvFjbs2UTpo3solVhvXSqL6lKDZ5P/ia?=
 =?us-ascii?Q?HG5CIrKvyH7ZgmYhw1h2YzfjeiJ/UOH6PDVznj0S+WeL1sD07xnviwidW5te?=
 =?us-ascii?Q?HeRf4+6E4issdmBUwqoEsR7uurYLa8JlLSVMR+ws+XxSLV4qoce4DccINbNN?=
 =?us-ascii?Q?dtw8QnSK7cwe4dgUWAStM0vjDc//Fmz3D8st63y8yTnXaruaI6PiTp2L9aEj?=
 =?us-ascii?Q?Xdg7GVI4gORwF1BfHU6j/bgXitVLRJQeEZvsGYrcJExMUbM8CWYsMgXfD2hv?=
 =?us-ascii?Q?7Ai4XWkbe4DKNSayG44BnzmPaH8nEMpkFawO1hzKBIwUnVJRRaxWUOsLQizb?=
 =?us-ascii?Q?q8hOgDSgG8+1d/MnpTOl04BffzHRhhJrau1JoCZsk58AY5xVNiD10U37e261?=
 =?us-ascii?Q?clAVwyh4X0l8chgW00xspnnYKdWNRSAGhwvuyIvaUqCh9VB08RAAOb1bj1jv?=
 =?us-ascii?Q?M1T1VdnSzEXTKOCj2EswSPXLM5ORMorTzBcLwkVGtXI0wr0C5EU8qpZ73yGo?=
 =?us-ascii?Q?9IYD58vH4vi6d8WijvPZ2FaU8wSMnCyMHzRCjG+UYNm1r6zvz4zsTSu7wUF2?=
 =?us-ascii?Q?ZqyE0mQgQLTo52tjSVDejLOnuJE/H++4L4LqWUclo43l3XG0pbbiLYWKPXlg?=
 =?us-ascii?Q?wY7DB4aC6bbkkq2HkcF3WhYt0zDjRs1BuKywekzxrMyoiDb0grwMk5WIKo0W?=
 =?us-ascii?Q?MhWDmgirOtsdPJbl0pYvwD7rNZaXsERXkkaA4R8kXz6TyLBRsww79zySua0/?=
 =?us-ascii?Q?5c6kLGkUkC6G7xRxL/fALd6I/H5u8U3G8EK5s103dcFoMx+cmhLIOvOxLxvk?=
 =?us-ascii?Q?CjyQPdzacKY8R2TK0UWJjDmgBS8GF2GsEDnhvFVQ6xbmyTU2mdwq7/GkVUFC?=
 =?us-ascii?Q?o0SM+3dmYyds3+N4vEn2KC7+wKQTpxo=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b2a0a129-94ca-402e-9cfc-08da3952be97
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 May 2022 04:47:57.5930
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QdmQe571GQtXRSHc+uqR0BWiGG89UPp/Za0h4/YuRI+WP4dX5sl5iVdd8I8Dy+/DucwBWLRHYs9BTOGWv/nRmA1f1Vkg1mwIWmNGdPrFsu4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR1001MB2149
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.874
 definitions=2022-05-18_09:2022-05-17,2022-05-18 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 spamscore=0 mlxscore=0
 phishscore=0 suspectscore=0 mlxlogscore=851 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2205190029
X-Proofpoint-GUID: Kfs1dEu2uuhuEdHQ-SQ8jdGkwX66lfah
X-Proofpoint-ORIG-GUID: Kfs1dEu2uuhuEdHQ-SQ8jdGkwX66lfah
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, May 19, 2022 at 01:11:56AM +0300, Pavel Skripkin wrote:
> diff --git a/drivers/staging/r8188eu/hal/rtl8188e_hal_init.c b/drivers/staging/r8188eu/hal/rtl8188e_hal_init.c
> index e67ecbd1ba79..22661c66cc18 100644
> --- a/drivers/staging/r8188eu/hal/rtl8188e_hal_init.c
> +++ b/drivers/staging/r8188eu/hal/rtl8188e_hal_init.c
> @@ -249,11 +249,14 @@ static void efuse_read_phymap_from_txpktbuf(
>  		hi32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H));
>  
>  		if (i == 0) {
> +			int res;
> +			u16 reg;
>  			/* Although lenc is only used in a debug statement,

Blank line after declarations.

I think it's better to put "int res" declarations at the start of the
function.  That's where people will expect to see it.

>  			 * do not remove it as the rtw_read16() call consumes
>  			 * 2 bytes from the EEPROM source.
>  			 */
> -			rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L);
> +			res = rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L, &reg);
> +			(void) res;
>  
>  			len = le32_to_cpu(lo32) & 0x0000ffff;
>  

[ snip ]

> diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
> index 1198d3850a6d..ce3369e33d66 100644
> --- a/drivers/staging/r8188eu/include/rtw_io.h
> +++ b/drivers/staging/r8188eu/include/rtw_io.h
> @@ -221,7 +221,7 @@ void _rtw_attrib_read(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
>  void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
>  
>  int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data);
> -u16 rtw_read16(struct adapter *adapter, u32 addr);
> +int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data);
>  u32 rtw_read32(struct adapter *adapter, u32 addr);
>  void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
>  u32 rtw_read_port(struct adapter *adapter, u8 *pmem);
> diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> index 66aac2cbe3a9..1b35951a53cb 100644
> --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> @@ -1919,7 +1919,10 @@ static int rtw_wx_read32(struct net_device *dev,
>  		sprintf(extra, "0x%02X", data32 & 0xff);
>  		break;
>  	case 2:
> -		data32 = rtw_read16(padapter, addr);
> +		ret = rtw_read16(padapter, addr, (u16 *) &data32);

Checkpatch.

I have an unpublished Smatch warning for casts like this.  You can't
pass a data32 pointer to something which is takes a u16 pointer and
expect it to work.  The last two bytes are uninitialized.

And even if you zero out the bytes, it is a bug on big endian systems.

> +		if (ret)
> +			goto err_free_ptmp;
> +
>  		sprintf(extra, "0x%04X", data32);
>  		break;
>  	case 4:

regards,
dan carpenter