Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 6/6] soc: fsl: qe: fix sparse warnings for ucc_slow.c
To:     Li Yang <leoyang.li@nxp.com>, Timur Tabi <timur@kernel.org>,
        Zhao Qiang <qiang.zhao@nxp.com>
Cc:     linux-arm-kernel@lists.infradead.org,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
References: <20200312222827.17409-1-leoyang.li@nxp.com>
 <20200312222827.17409-7-leoyang.li@nxp.com>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <b9c5a514-18c1-e36c-1595-2b86c9bfcff1@rasmusvillemoes.dk>
Date:   Mon, 16 Mar 2020 22:07:25 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20200312222827.17409-7-leoyang.li@nxp.com>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 12/03/2020 23.28, Li Yang wrote:
> Fixes the following sparse warnings:
> 
[snip]
> 
> Also removed the unneccessary clearing for kzalloc'ed structure.

Please don't mix that in the same patch, do it in a preparatory patch.
That makes reviewing much easier.

>  
>  	/* Get PRAM base */
>  	uccs->us_pram_offset =
> @@ -231,24 +224,24 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc
>  		/* clear bd buffer */
>  		qe_iowrite32be(0, &bd->buf);
>  		/* set bd status and length */
> -		qe_iowrite32be(0, (u32 *)bd);
> +		qe_iowrite32be(0, (u32 __iomem *)bd);

It's cleaner to do two qe_iowrite16be to &bd->status and &bd->length,
that avoids the casting altogether.

>  		bd++;
>  	}
>  	/* for last BD set Wrap bit */
>  	qe_iowrite32be(0, &bd->buf);
> -	qe_iowrite32be(cpu_to_be32(T_W), (u32 *)bd);
> +	qe_iowrite32be(T_W, (u32 __iomem *)bd);

Yeah, and this is why. Who can actually keep track of where that bit
ends up being set with that casting going on. Please use
qe_iowrite16be() with an appropriately modified constant to the
appropriate field instead of these games.

And if the hardware doesn't support 16 bit writes, the definition of
struct qe_bd is wrong and should have a single __be32 status_length
field, with appropriate accessors defined.

>  	/* Init Rx bds */
>  	bd = uccs->rx_bd = qe_muram_addr(uccs->rx_base_offset);
>  	for (i = 0; i < us_info->rx_bd_ring_len - 1; i++) {
>  		/* set bd status and length */
> -		qe_iowrite32be(0, (u32 *)bd);
> +		qe_iowrite32be(0, (u32 __iomem *)bd);

Same.

>  		/* clear bd buffer */
>  		qe_iowrite32be(0, &bd->buf);
>  		bd++;
>  	}
>  	/* for last BD set Wrap bit */
> -	qe_iowrite32be(cpu_to_be32(R_W), (u32 *)bd);
> +	qe_iowrite32be(R_W, (u32 __iomem *)bd);

Same.

>  	qe_iowrite32be(0, &bd->buf);
>  
>  	/* Set GUMR (For more details see the hardware spec.). */
> @@ -273,8 +266,8 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc
>  	qe_iowrite32be(gumr, &us_regs->gumr_h);
>  
>  	/* gumr_l */
> -	gumr = us_info->tdcr | us_info->rdcr | us_info->tenc | us_info->renc |
> -		us_info->diag | us_info->mode;
> +	gumr = (u32)us_info->tdcr | (u32)us_info->rdcr | (u32)us_info->tenc |
> +	       (u32)us_info->renc | (u32)us_info->diag | (u32)us_info->mode;

Are the tdcr, rdcr, tenc, renc fields actually set anywhere (the same
for the diag and mode, but word-grepping for those give way too many
false positives)? They seem to be a somewhat pointless split out of the
bitfields of gumr_l, and not populated anywhere?. That's not directly
related to this patch, of course, but getting rid of them first (if they
are indeed completely unused) might make the sparse cleanup a little
simpler.

Rasmus