Re: [PATCH 1/6] scsi: bfa: use min_t() to improve code

[David Laight <david.laight.linux@gmail.com>]

On Fri, 15 Aug 2025 20:16:03 +0800
Qianfeng Rong <rongqianfeng@vivo.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@vivo.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) {