Re: [RFC net-next 10/17] net/dibs: Define dibs_client_ops and dibs_dev_ops

[Simon Horman <horms@kernel.org>]

On Wed, Aug 06, 2025 at 05:41:15PM +0200, Alexandra Winter wrote:

...

> diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c

...

> -static void smcd_register_dev(struct ism_dev *ism)
> +static void smcd_register_dev(struct dibs_dev *dibs)
>  {
> -	const struct smcd_ops *ops = ism_get_smcd_ops();
>  	struct smcd_dev *smcd, *fentry;
> +	const struct smcd_ops *ops;
> +	struct smc_lo_dev *smc_lo;
> +	struct ism_dev *ism;
>  
> -	if (!ops)
> -		return;
> +	if (smc_ism_is_loopback(dibs)) {
> +		if (smc_loopback_init(&smc_lo))
> +			return;
> +	}
>  
> -	smcd = smcd_alloc_dev(&ism->pdev->dev, dev_name(&ism->pdev->dev), ops,
> -			      ISM_NR_DMBS);
> +	if (smc_ism_is_loopback(dibs)) {
> +		ops = smc_lo_get_smcd_ops();
> +		smcd = smcd_alloc_dev(dev_name(&smc_lo->dev), ops,
> +				      SMC_LO_MAX_DMBS);
> +	} else {
> +		ism = dibs->drv_priv;
> +		ops = ism_get_smcd_ops();
> +		smcd = smcd_alloc_dev(dev_name(&ism->pdev->dev), ops,
> +				      ISM_NR_DMBS);
> +	}

Hi Alexandra,

ism is initialised conditionally here.

But towards the end of this function the following dereferences
ism unconditionally. And it's not clear to me this won't occur
even if ism wasn't initialised above.

        if (smc_pnet_is_pnetid_set(smcd->pnetid))
                pr_warn_ratelimited("smc: adding smcd device %s with pnetid %.16s%s\n",
                                    dev_name(&ism->dev), smcd->pnetid,
                                    smcd->pnetid_by_user ?
                                        " (user defined)" :
                                        "");
        else
                pr_warn_ratelimited("smc: adding smcd device %s without pnetid\n",
                                    dev_name(&ism->dev));


>  	if (!smcd)
>  		return;
> -	smcd->priv = ism;
> +
> +	smcd->dibs = dibs;
> +	dibs_set_priv(dibs, &smc_dibs_client, smcd);
> +
> +	if (smc_ism_is_loopback(dibs)) {
> +		smcd->priv = smc_lo;
> +		smc_lo->smcd = smcd;
> +	} else {
> +		smcd->priv = ism;
> +		ism_set_priv(ism, &smc_ism_client, smcd);

This function is now compiled even if CONFIG_ISM is not enabled.
But smc_ism_client is only defined if CONFIG_ISM is enabled.

I think this code is removed by later patches. But nonetheless
I also think this leads to a build error and it's best
to avoid transient build errors as they break bisection.

> +		if (smc_pnetid_by_dev_port(&ism->pdev->dev, 0, smcd->pnetid))
> +			smc_pnetid_by_table_smcd(smcd);
> +	}
> +
>  	smcd->client = &smc_ism_client;

Ditto.

> -	ism_set_priv(ism, &smc_ism_client, smcd);
> -	if (smc_pnetid_by_dev_port(&ism->pdev->dev, 0, smcd->pnetid))
> -		smc_pnetid_by_table_smcd(smcd);
>  
>  	if (smcd->ops->supports_v2())
>  		smc_ism_set_v2_capable();

...