linux-kernel - Re: [PATCH mfd 1/1] mfd/ptp: clockmatrix: support 32-bit address space

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: <20230322125619.731912cf@kernel.org>
Date:   Wed, 22 Mar 2023 12:56:19 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Min Li <lnimi@...mail.com>
Cc:     richardcochran@...il.com, lee@...nel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Min Li <min.li.xe@...esas.com>
Subject: Re: [PATCH mfd 1/1] mfd/ptp: clockmatrix: support 32-bit address
 space

On Tue, 21 Mar 2023 15:10:06 -0400 Min Li wrote:
> -		buf[0] = (u8)(page & 0xff);
> -		buf[1] = (u8)((page >> 8) & 0xff);
> +		buf[0] = (u8)(page & 0xFF);
> +		buf[1] = (u8)((page >> 8) & 0xFF);

why did you decide to change from 0xff to 0xFF as part of this big
change? It's unnecessary churn.

> +		buf[2] = (u8)((page >> 16) & 0xFF);
> +		buf[3] = (u8)((page >> 24) & 0xFF);
> +		bytes = 4;
>  		break;

>  static inline int idtcm_read(struct idtcm *idtcm,
> -			     u16 module,
> +			     u32 module,
>  			     u16 regaddr,
>  			     u8 *buf,
>  			     u16 count)
> @@ -50,7 +50,7 @@ static inline int idtcm_read(struct idtcm *idtcm,
>  }
>  
>  static inline int idtcm_write(struct idtcm *idtcm,
> -			      u16 module,
> +			      u32 module,
>  			      u16 regaddr,
>  			      u8 *buf,
>  			      u16 count)

>  	/* PLL5 can have OUT8 as second additional output. */
>  	if (pll == 5 && qn_plus_1 != 0) {
> -		err = idtcm_read(idtcm, 0, HW_Q8_CTRL_SPARE,
> +		err = idtcm_read(idtcm, HW_Q8_CTRL_SPARE, 0,
>  				 &temp, sizeof(temp));
>  		if (err)
>  			return err;
>  
>  		temp &= ~(Q9_TO_Q8_SYNC_TRIG);
>  
> -		err = idtcm_write(idtcm, 0, HW_Q8_CTRL_SPARE,
> +		err = idtcm_write(idtcm, HW_Q8_CTRL_SPARE, 0,
>  				  &temp, sizeof(temp));
>  		if (err)
>  			return err;
>  
>  		temp |= Q9_TO_Q8_SYNC_TRIG;
>  
> -		err = idtcm_write(idtcm, 0, HW_Q8_CTRL_SPARE,
> +		err = idtcm_write(idtcm, HW_Q8_CTRL_SPARE, 0,
>  				  &temp, sizeof(temp));
>  		if (err)
>  			return err;

Why are you flipping all these arguments?
Isn't HW_Q8_CTRL_SPARE regaddr?

Could you to split your patches into multiple steps to make them easier
to reivew?