linux-kernel - Re: [PATCH net-next v7 1/5] 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] [day] [month] [year] [list]

Message-ID: <8a06f42e3a7028f88920764d5a70637a6a174eac.camel@redhat.com>
Date: Tue, 09 Jan 2024 08:46:17 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Min Li <lnimi@...mail.com>, richardcochran@...il.com, lee@...nel.org
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org, Min Li
	 <min.li.xe@...esas.com>
Subject: Re: [PATCH net-next v7 1/5] ptp: clockmatrix: support 32-bit
 address space

On Thu, 2024-01-04 at 11:36 -0500, Min Li wrote:
> @@ -553,11 +554,11 @@ static int _sync_pll_output(struct idtcm *idtcm,
>  	val = SYNCTRL1_MASTER_SYNC_RST;
>  
>  	/* Place master sync in reset */
> -	err = idtcm_write(idtcm, 0, sync_ctrl1, &val, sizeof(val));
> +	err = idtcm_write(idtcm, sync_ctrl1, 0, &val, sizeof(val));
>  	if (err)
>  		return err;

I'm sorry for the late feedback: I lost track the last replies in the
previous revision and later I was on PTO.

Let me extract the relevant slice from such thread:

On  Wed, 13 Dec 2023 21:04:07 +0000 Min Li wrote:
> > My reading is that this patch reverses the usage of module and regaddr.
> > F.e. the following hunk:
> > 
> > @@ -553,11 +554,11 @@ static int _sync_pll_output(struct idtcm *idtcm,
> > 	val = SYNCTRL1_MASTER_SYNC_RST;
> > 
> > 	/* Place master sync in reset */
> > 	err = idtcm_write(idtcm, 0, sync_ctrl1, &val, sizeof(val));
> > 	err = idtcm_write(idtcm, sync_ctrl1, 0, &val, sizeof(val));
> > 	if (err)
> > 		return err;
> > 
> > 	err = idtcm_write(idtcm, 0, sync_ctrl0, &sync_src, sizeof(sync_src));
> > 	err = idtcm_write(idtcm, sync_ctrl0, 0, &sync_src, sizeof(sync_src));
> > 	if (err)
> > 		return err;
> > 
> > If that is really intended I think it needs to be explained, or possibly a
> > separate patch.
>
> Hi Simon
> sync_ctrl0/1 was meant to be a module and it was in a wrong place. 
> And this patch is just correcting it.

Then you need to move this chunk (and all the later on swapping the
'address' and the 'module' argument in a separate patch. Mixing this
fix and  the address space extension is confusing.

Additionally we are currently preparing the net-next PR for 6.8 and I
don't feel very confident to apply such a large refactor this late. I
think it should be better postpone to the next cycle.

Cheers,

Paolo