linux-kernel - Re: [PATCH 1/6] scsi: bfa: use min

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: <20250820131303.1fa8e046@pumpkin>
Date: Wed, 20 Aug 2025 13:13:03 +0100
From: David Laight <david.laight.linux@...il.com>
To: Qianfeng Rong <rongqianfeng@...o.com>
Cc: Anil Gurumurthy <anil.gurumurthy@...gic.com>, Sudarsana Kalluru
 <sudarsana.kalluru@...gic.com>, "James E.J. Bottomley"
 <James.Bottomley@...senPartnership.com>, "Martin K. Petersen"
 <martin.petersen@...cle.com>, linux-scsi@...r.kernel.org (open list:BROCADE
 BFA FC SCSI DRIVER), linux-kernel@...r.kernel.org (open list)
Subject: Re: [PATCH 1/6] scsi: bfa: use min_t() to improve code

On Fri, 15 Aug 2025 20:16:03 +0800
Qianfeng Rong <rongqianfeng@...o.com> wrote:

> Use min_t() to reduce the code in bfa_fcs_rport_update() and
> bfa_sgpg_mfree(), and improve readability.
> 
> Signed-off-by: Qianfeng Rong <rongqianfeng@...o.com>
> ---
>  drivers/scsi/bfa/bfa_fcs_rport.c | 8 +++-----
>  drivers/scsi/bfa/bfa_svc.c       | 5 +----
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/bfa/bfa_fcs_rport.c b/drivers/scsi/bfa/bfa_fcs_rport.c
> index d4bde9bbe75b..77dc7aaf5985 100644
> --- a/drivers/scsi/bfa/bfa_fcs_rport.c
> +++ b/drivers/scsi/bfa/bfa_fcs_rport.c
> @@ -11,7 +11,6 @@
>  /*
>   *  rport.c Remote port implementation.
>   */
> -
>  #include "bfad_drv.h"
>  #include "bfad_im.h"
>  #include "bfa_fcs.h"
> @@ -2555,10 +2554,9 @@ bfa_fcs_rport_update(struct bfa_fcs_rport_s *rport, struct fc_logi_s *plogi)
>  	 * - MAX receive frame size
>  	 */
>  	rport->cisc = plogi->csp.cisc;
> -	if (be16_to_cpu(plogi->class3.rxsz) < be16_to_cpu(plogi->csp.rxsz))
> -		rport->maxfrsize = be16_to_cpu(plogi->class3.rxsz);
> -	else
> -		rport->maxfrsize = be16_to_cpu(plogi->csp.rxsz);
> +	rport->maxfrsize = min_t(typeof(rport->maxfrsize),
> +				 be16_to_cpu(plogi->class3.rxsz),
> +				 be16_to_cpu(plogi->csp.rxsz));

I think I want to nak that one.
If you are going to use min_t() the type has to be one than includes
all possible values of both arguments.
Using the type of the result is just plain wrong.
There is also pretty much no point casting the values to char/short types
unless you need the implicit masking.
The values are immediately promoted to 'signed int' before the comparison.
I also think that min() will accept an 'unsigned char/short' variable
for a comparison against a 'signed int'.

So, all in all, min() should be fine.
Avoiding the extra be16_to_cpu() is probably a gain.
The compiler may not always know the value doesn't change.

	David

>  
>  	bfa_trc(port->fcs, be16_to_cpu(plogi->csp.bbcred));
>  	bfa_trc(port->fcs, port->fabric->bb_credit);
> diff --git a/drivers/scsi/bfa/bfa_svc.c b/drivers/scsi/bfa/bfa_svc.c
> index df33afaaa673..2570793aae7f 100644
> --- a/drivers/scsi/bfa/bfa_svc.c
> +++ b/drivers/scsi/bfa/bfa_svc.c
> @@ -5202,10 +5202,7 @@ bfa_sgpg_mfree(struct bfa_s *bfa, struct list_head *sgpg_q, int nsgpg)
>  	 */
>  	do {
>  		wqe = bfa_q_first(&mod->sgpg_wait_q);
> -		if (mod->free_sgpgs < wqe->nsgpg)
> -			nsgpg = mod->free_sgpgs;
> -		else
> -			nsgpg = wqe->nsgpg;
> +		nsgpg = min_t(int, mod->free_sgpgs, wqe->nsgpg);
>  		bfa_sgpg_malloc(bfa, &wqe->sgpg_q, nsgpg);
>  		wqe->nsgpg -= nsgpg;
>  		if (wqe->nsgpg == 0) {