Re: [RFC net-next 03/17] net/smc: Remove error handling of unregister_dmb()

[Alexandra Winter <wintera@linux.ibm.com>]

On 10.08.25 13:03, Dust Li wrote:
> On 2025-08-06 17:41:08, Alexandra Winter wrote:
>> smcd_buf_free() calls smc_ism_unregister_dmb(lgr->smcd, buf_desc) and
>> then unconditionally frees buf_desc.
>>
>> Remove the cleaning up of fields of buf_desc in
>> smc_ism_unregister_dmb(), because it is not helpful.
>>
>> This removes the only usage of ISM_ERROR from the smc module. So move it
>> to drivers/s390/net/ism.h.
>>
>> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
>> Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>> ---
>> drivers/s390/net/ism.h |  1 +
>> include/net/smc.h      |  2 --
>> net/smc/smc_ism.c      | 14 +++++---------
>> net/smc/smc_ism.h      |  3 ++-
>> 4 files changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/s390/net/ism.h b/drivers/s390/net/ism.h
>> index 047fa6101555..b5b03db52fce 100644
>> --- a/drivers/s390/net/ism.h
>> +++ b/drivers/s390/net/ism.h
>> @@ -10,6 +10,7 @@
>> #include <asm/pci_insn.h>
>>
>> #define UTIL_STR_LEN	16
>> +#define ISM_ERROR	0xFFFF
>>
>> /*
>>  * Do not use the first word of the DMB bits to ensure 8 byte aligned access.
>> diff --git a/include/net/smc.h b/include/net/smc.h
>> index db84e4e35080..a9c023dd1380 100644
>> --- a/include/net/smc.h
>> +++ b/include/net/smc.h
>> @@ -44,8 +44,6 @@ struct smcd_dmb {
>>
>> #define ISM_RESERVED_VLANID	0x1FFF
>>
>> -#define ISM_ERROR	0xFFFF
>> -
>> struct smcd_dev;
>>
>> struct smcd_gid {
>> diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c
>> index 84f98e18c7db..a94e1450d095 100644
>> --- a/net/smc/smc_ism.c
>> +++ b/net/smc/smc_ism.c
>> @@ -205,13 +205,13 @@ int smc_ism_put_vlan(struct smcd_dev *smcd, unsigned short vlanid)
>> 	return rc;
>> }
>>
>> -int smc_ism_unregister_dmb(struct smcd_dev *smcd, struct smc_buf_desc *dmb_desc)
>> +void smc_ism_unregister_dmb(struct smcd_dev *smcd,
>> +			    struct smc_buf_desc *dmb_desc)
>> {
>> 	struct smcd_dmb dmb;
>> -	int rc = 0;
>>
>> 	if (!dmb_desc->dma_addr)
>> -		return rc;
>> +		return;
>>
>> 	memset(&dmb, 0, sizeof(dmb));
>> 	dmb.dmb_tok = dmb_desc->token;
>> @@ -219,13 +219,9 @@ int smc_ism_unregister_dmb(struct smcd_dev *smcd, struct smc_buf_desc *dmb_desc)
>> 	dmb.cpu_addr = dmb_desc->cpu_addr;
>> 	dmb.dma_addr = dmb_desc->dma_addr;
>> 	dmb.dmb_len = dmb_desc->len;
>> -	rc = smcd->ops->unregister_dmb(smcd, &dmb);
>> -	if (!rc || rc == ISM_ERROR) {
>> -		dmb_desc->cpu_addr = NULL;
>> -		dmb_desc->dma_addr = 0;
>> -	}
>> +	smcd->ops->unregister_dmb(smcd, &dmb);
> 
> Hmm, I think the old way of handling error here is certainly not good.
> But completely ignoring error handling here would make bugs harder
> to detect.
> 
> What about adding a WARN_ON_ONCE(rc) ?
> 
> Also, I think we can just remove the rc == ISM_ERROR to remove
> the dependency of ISM_ERROR in smc.
> 
> Best regards,
> Dust
> 

As I wrote in the commit message, I removed rc, because it is ignored by the caller anyhow today.
If you want to I can add it back into this function and then you can think about how SMC should
handle such an error.

My thoughts on this are:
There is not really much smc can do about a problem in unregister_dmb.

I think it is Linux strategy to report and handle errors at the lowest level possible. I have some
patches on my harddisk to improve error handling and error reporting of the ism device diver. And
we are already discussing internally which errors should do a WARN_ON_ONCE in the ism device driver.

So I don't think there is much to do in the smc layer at the moment.
How does that sound to you?