linux-kernel - Re: [net-next v7 4/7] net: mtip: The L2 switch driver for imx287

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fc450dca-a1ba-4b9f-befa-f9643d9b1b82@kernel.org>
Date: Fri, 25 Apr 2025 10:08:52 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Lukasz Majewski <lukma@...x.de>
Cc: Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>,
 davem@...emloft.net, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>,
 Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 Stefan Wahren <wahrenst@....net>, Simon Horman <horms@...nel.org>,
 Andrew Lunn <andrew@...n.ch>
Subject: Re: [net-next v7 4/7] net: mtip: The L2 switch driver for imx287

On 25/04/2025 09:49, Lukasz Majewski wrote:
> Hi Krzysztof, Jakub
> 
>> On 25/04/2025 08:05, Lukasz Majewski wrote:
>>> Hi Krzysztof, Jakub,
>>>   
>>>> On 25/04/2025 03:11, Jakub Kicinski wrote:  
>>>>> On Wed, 23 Apr 2025 09:29:08 +0200 Lukasz Majewski wrote:    
>>>>>> This patch series provides support for More Than IP L2 switch
>>>>>> embedded in the imx287 SoC.
>>>>>>
>>>>>> This is a two port switch (placed between uDMA[01] and
>>>>>> MAC-NET[01]), which can be used for offloading the network
>>>>>> traffic.
>>>>>>
>>>>>> It can be used interchangeably with current FEC driver - to be
>>>>>> more specific: one can use either of it, depending on the
>>>>>> requirements.
>>>>>>
>>>>>> The biggest difference is the usage of DMA - when FEC is used,
>>>>>> separate DMAs are available for each ENET-MAC block.
>>>>>> However, with switch enabled - only the DMA0 is used to
>>>>>> send/receive data to/form switch (and then switch sends them to
>>>>>> respecitive ports).    
>>>>>
>>>>> Lots of sparse warnings and build issues here, at least on x86.
>>>>>
>>>>> Could you make sure it's clean with an allmodconfig config, 
>>>>> something like:
>>>>>
>>>>> make C=1 W=1 drivers/net/ethernet/freescale/mtipsw/     
>>>>
>>>> ... and W=1 with clang as well.
>>>>  
>>>
>>> The sparse warnings are because of struct switch_t casting and
>>> register  
>>
>> clang W=1 fails on errors, so it is not only sparse:
>>
>> error: cast to smaller integer type 'uint' (aka 'unsigned int') from
>> 'struct cbd_t *' [-Werror,-Wpointer-to-int-cast]
>>
>> You probably wanted there kenel_ulong_t.
> 
> This I did not catch earlier (probably because of my testing on
> imx287). Thanks for spotting it.
> 
>>
>>> access with this paradigm (as it is done with other drivers).  
>>
>> I don't understand. I see code like:
>>
>> 	struct switch_t *fecp = fep->hwp;
>>
>> But this is not a cast - the same types.
> 
> For example:
> 
> The warning:
> 
> mtipl2sw.c:208:30: warning: incorrect type in argument 1 (different
> address spaces) mtipl2sw.c:208:30:    expected void const volatile
> [noderef] __iomem *addr mtipl2sw.c:208:30:    got unsigned int *
> 
> corresponds to:
>  info->maclo = readl(&fecp->ESW_LREC0);   [*]
> 
> where:
> 
> struct switch_t {
>         u32 ESW_REVISION;
>         u32 ESW_SCRATCH;
> 	...
>         /*from 0x420-0x4FC*/
>         u32 esw_reserved9[57];
>         /*0xFC0DC500---0xFC0DC508*/
>         u32 ESW_LREC0;
>         u32 ESW_LREC1;
>         u32 ESW_LSR;
> };
> 
> 
> The 'u32' type seems to be valid here as this register is 32 bit wide.

It is not about size, but IOMEM annotation and pointer/non-pointer.


> 
> To fix the sparse warnings - I think that I will replace [*] with:
> 
> info->maclo = readl((u32 __iomem *)&fecp->ESW_LREC0);

I don't understand why are you reading address of ESW_LREC0. This is
MMIO, right? So you are supposes to read base + offset (where base is a
proper iomem pointer).


Best regards,
Krzysztof