This implements some operations that loopback-ism does not support
currently:

- vlan operations, since there is no strong use-case for it.
- signal_event operations, since there is no event to be processed by
  the loopback-ism device.

Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/smc_loopback.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/net/smc/smc_loopback.c b/net/smc/smc_loopback.c
index 3eb623e030eb..4b5e864ebca3 100644
--- a/net/smc/smc_loopback.c
+++ b/net/smc/smc_loopback.c
@@ -49,6 +49,32 @@ static int smc_lo_query_rgid(struct smcd_dev *smcd, struct smcd_gid *rgid,
 	return 0;
 }
 
+static int smc_lo_add_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
+{
+	return -EOPNOTSUPP;
+}
+
+static int smc_lo_del_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
+{
+	return -EOPNOTSUPP;
+}
+
+static int smc_lo_set_vlan_required(struct smcd_dev *smcd)
+{
+	return -EOPNOTSUPP;
+}
+
+static int smc_lo_reset_vlan_required(struct smcd_dev *smcd)
+{
+	return -EOPNOTSUPP;
+}
+
+static int smc_lo_signal_event(struct smcd_dev *dev, struct smcd_gid *rgid,
+			       u32 trigger_irq, u32 event_code, u64 info)
+{
+	return 0;
+}
+
 static int smc_lo_supports_v2(void)
 {
 	return SMC_LO_V2_CAPABLE;
@@ -77,11 +103,11 @@ static const struct smcd_ops lo_ops = {
 	.query_remote_gid = smc_lo_query_rgid,
 	.register_dmb		= NULL,
 	.unregister_dmb		= NULL,
-	.add_vlan_id		= NULL,
-	.del_vlan_id		= NULL,
-	.set_vlan_required	= NULL,
-	.reset_vlan_required	= NULL,
-	.signal_event		= NULL,
+	.add_vlan_id = smc_lo_add_vlan_id,
+	.del_vlan_id = smc_lo_del_vlan_id,
+	.set_vlan_required = smc_lo_set_vlan_required,
+	.reset_vlan_required = smc_lo_reset_vlan_required,
+	.signal_event = smc_lo_signal_event,
 	.move_data		= NULL,
 	.supports_v2 = smc_lo_supports_v2,
 	.get_local_gid = smc_lo_get_local_gid,
-- 
2.32.0.3.g01195cf9f

On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
> This implements some operations that loopback-ism does not support
> currently:
>  - vlan operations, since there is no strong use-case for it.
>  - signal_event operations, since there is no event to be processed 
> by the loopback-ism device.

Hi Wen,

I wonder if the these operations that are not supported by loopback-ism
should rather be marked "optional" in the struct smcd_ops, and the
calling code should call these only when they are implemented.

Of course this would mean more changes to net/smc/smc_core.c - but
loopback-ism could omit these "boiler-plate" functions.

>  
> +static int smc_lo_add_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int smc_lo_del_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int smc_lo_set_vlan_required(struct smcd_dev *smcd)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int smc_lo_reset_vlan_required(struct smcd_dev *smcd)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int smc_lo_signal_event(struct smcd_dev *dev, struct smcd_gid
> *rgid,
> +			       u32 trigger_irq, u32 event_code, u64
> info)
> +{
> +	return 0;
> +}
> +

Just a pattern that I saw elsewhere in the kernel...

Thanks,
Gerd

On 2024/4/4 00:25, Gerd Bayer wrote:
> On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
>> This implements some operations that loopback-ism does not support
>> currently:
>>   - vlan operations, since there is no strong use-case for it.
>>   - signal_event operations, since there is no event to be processed
>> by the loopback-ism device.
> 
> Hi Wen,
> 
> I wonder if the these operations that are not supported by loopback-ism
> should rather be marked "optional" in the struct smcd_ops, and the
> calling code should call these only when they are implemented.
> 
> Of course this would mean more changes to net/smc/smc_core.c - but
> loopback-ism could omit these "boiler-plate" functions.
> 

Hi Gerd.

Thank you for the thoughts! I agree that checks like 'if(smcd->ops->xxx)'
can avoid the device driver from implementing unsupported operations. But I
am afraid that which operations need to be defined as 'optional' may differ
from different device perspectives (e.g. for loopback-ism they are vlan-related
opts and signal_event). So I perfer to simply let the smc protocol assume
that all operations have been implemented, and let drivers to decide which
ones are unsupported in implementation. What do you think?

Thanks!

>>   
>> +static int smc_lo_add_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int smc_lo_del_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int smc_lo_set_vlan_required(struct smcd_dev *smcd)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int smc_lo_reset_vlan_required(struct smcd_dev *smcd)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int smc_lo_signal_event(struct smcd_dev *dev, struct smcd_gid
>> *rgid,
>> +			       u32 trigger_irq, u32 event_code, u64
>> info)
>> +{
>> +	return 0;
>> +}
>> +
> 
> Just a pattern that I saw elsewhere in the kernel...
> 
> Thanks,
> Gerd

On Thu, 2024-04-04 at 17:32 +0800, Wen Gu wrote:
> 
> On 2024/4/4 00:25, Gerd Bayer wrote:
> > On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
> > > This implements some operations that loopback-ism does not support
> > > currently:
> > >   - vlan operations, since there is no strong use-case for it.
> > >   - signal_event operations, since there is no event to be processed
> > > by the loopback-ism device.
> > 
> > Hi Wen,
> > 
> > I wonder if the these operations that are not supported by loopback-ism
> > should rather be marked "optional" in the struct smcd_ops, and the
> > calling code should call these only when they are implemented.
> > 
> > Of course this would mean more changes to net/smc/smc_core.c - but
> > loopback-ism could omit these "boiler-plate" functions.
> > 
> 
> Hi Gerd.
> 
> Thank you for the thoughts! I agree that checks like 'if(smcd->ops->xxx)'
> can avoid the device driver from implementing unsupported operations. But I
> am afraid that which operations need to be defined as 'optional' may differ
> from different device perspectives (e.g. for loopback-ism they are vlan-related
> opts and signal_event). So I perfer to simply let the smc protocol assume
> that all operations have been implemented, and let drivers to decide which
> ones are unsupported in implementation. What do you think?
> 
> Thanks!
> 

I agree with Gerd, in my opinion it is better to document ops as
optional and then allow their function pointers to be NULL and check
for that. Acting like they are supported and then they turn out to be
nops to me seems to contradict the principle of least surprises. I also
think we can find a subset of mandatory ops without which SMC-D is
impossible and then everything else should be optional.

As a first guess I think the following options may be mandatory:

* query_remote_gid()
* register_dmb()/unregister_dmb()
* move_data()
  For this one could argue that either move_data() or
  attach_dmb()/detach_dmb() is required though personally I would
  prefer to always have move_data() as a fallback and simple API
* supports_v2()
* get_local_gid()
* get_chid()
* get_dev()
> > 

On 2024/4/4 19:42, Niklas Schnelle wrote:
> On Thu, 2024-04-04 at 17:32 +0800, Wen Gu wrote:
>>
>> On 2024/4/4 00:25, Gerd Bayer wrote:
>>> On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
>>>> This implements some operations that loopback-ism does not support
>>>> currently:
>>>>    - vlan operations, since there is no strong use-case for it.
>>>>    - signal_event operations, since there is no event to be processed
>>>> by the loopback-ism device.
>>>
>>> Hi Wen,
>>>
>>> I wonder if the these operations that are not supported by loopback-ism
>>> should rather be marked "optional" in the struct smcd_ops, and the
>>> calling code should call these only when they are implemented.
>>>
>>> Of course this would mean more changes to net/smc/smc_core.c - but
>>> loopback-ism could omit these "boiler-plate" functions.
>>>
>>
>> Hi Gerd.
>>
>> Thank you for the thoughts! I agree that checks like 'if(smcd->ops->xxx)'
>> can avoid the device driver from implementing unsupported operations. But I
>> am afraid that which operations need to be defined as 'optional' may differ
>> from different device perspectives (e.g. for loopback-ism they are vlan-related
>> opts and signal_event). So I perfer to simply let the smc protocol assume
>> that all operations have been implemented, and let drivers to decide which
>> ones are unsupported in implementation. What do you think?
>>
>> Thanks!
>>
> 
> I agree with Gerd, in my opinion it is better to document ops as
> optional and then allow their function pointers to be NULL and check
> for that. Acting like they are supported and then they turn out to be
> nops to me seems to contradict the principle of least surprises. I also
> think we can find a subset of mandatory ops without which SMC-D is
> impossible and then everything else should be optional.

I see. If we all agree to classify smcd_ops into mandatory and optional ones,
I'll add a patch to mark the optional ops and check if they are implemented.

> 
> As a first guess I think the following options may be mandatory:
> 
> * query_remote_gid()
> * register_dmb()/unregister_dmb()
> * move_data()
>    For this one could argue that either move_data() or
>    attach_dmb()/detach_dmb() is required though personally I would
>    prefer to always have move_data() as a fallback and simple API
> * supports_v2()
> * get_local_gid()
> * get_chid()
> * get_dev()
I agree with this classification. Just one point, maybe we can take
supports_v2() as an optional ops, like support_dmb_nocopy()? e.g. if
it is not implemented, we treat it as an ISMv1.

Thanks!

>>>

On Thu, 2024-04-04 at 21:12 +0800, Wen Gu wrote:
> 
> On 2024/4/4 19:42, Niklas Schnelle wrote:
> > On Thu, 2024-04-04 at 17:32 +0800, Wen Gu wrote:
> > > 
> > > On 2024/4/4 00:25, Gerd Bayer wrote:
> > > > On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
> > > > > This implements some operations that loopback-ism does not support
> > > > > currently:
> > > > >    - vlan operations, since there is no strong use-case for it.
> > > > >    - signal_event operations, since there is no event to be processed
> > > > > by the loopback-ism device.
> > > > 
> > > > Hi Wen,
> > > > 
> > > > I wonder if the these operations that are not supported by loopback-ism
> > > > should rather be marked "optional" in the struct smcd_ops, and the
> > > > calling code should call these only when they are implemented.
> > > > 
> > > > Of course this would mean more changes to net/smc/smc_core.c - but
> > > > loopback-ism could omit these "boiler-plate" functions.
> > > > 
> > > 
> > > Hi Gerd.
> > > 
> > > Thank you for the thoughts! I agree that checks like 'if(smcd->ops->xxx)'
> > > can avoid the device driver from implementing unsupported operations. But I
> > > am afraid that which operations need to be defined as 'optional' may differ
> > > from different device perspectives (e.g. for loopback-ism they are vlan-related
> > > opts and signal_event). So I perfer to simply let the smc protocol assume
> > > that all operations have been implemented, and let drivers to decide which
> > > ones are unsupported in implementation. What do you think?
> > > 
> > > Thanks!
> > > 
> > 
> > I agree with Gerd, in my opinion it is better to document ops as
> > optional and then allow their function pointers to be NULL and check
> > for that. Acting like they are supported and then they turn out to be
> > nops to me seems to contradict the principle of least surprises. I also
> > think we can find a subset of mandatory ops without which SMC-D is
> > impossible and then everything else should be optional.
> 
> I see. If we all agree to classify smcd_ops into mandatory and optional ones,
> I'll add a patch to mark the optional ops and check if they are implemented.

Keep in mind I don't speak for the SMC maintainers but that does sound
reasonable to me.

> 
> > 
> > As a first guess I think the following options may be mandatory:
> > 
> > * query_remote_gid()
> > * register_dmb()/unregister_dmb()
> > * move_data()
> >    For this one could argue that either move_data() or
> >    attach_dmb()/detach_dmb() is required though personally I would
> >    prefer to always have move_data() as a fallback and simple API
> > * supports_v2()
> > * get_local_gid()
> > * get_chid()
> > * get_dev()
> I agree with this classification. Just one point, maybe we can take
> supports_v2() as an optional ops, like support_dmb_nocopy()? e.g. if
> it is not implemented, we treat it as an ISMv1.
> 
> Thanks!

Interpreting a NULL supports_v2() as not supporting v2 sounds
reasonable to me.

On 2024/4/4 23:15, Niklas Schnelle wrote:
> On Thu, 2024-04-04 at 21:12 +0800, Wen Gu wrote:
>>
>> On 2024/4/4 19:42, Niklas Schnelle wrote:
>>> On Thu, 2024-04-04 at 17:32 +0800, Wen Gu wrote:
>>>>
>>>> On 2024/4/4 00:25, Gerd Bayer wrote:
>>>>> On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
>>>>>> This implements some operations that loopback-ism does not support
>>>>>> currently:
>>>>>>     - vlan operations, since there is no strong use-case for it.
>>>>>>     - signal_event operations, since there is no event to be processed
>>>>>> by the loopback-ism device.
>>>>>
>>>>> Hi Wen,
>>>>>
>>>>> I wonder if the these operations that are not supported by loopback-ism
>>>>> should rather be marked "optional" in the struct smcd_ops, and the
>>>>> calling code should call these only when they are implemented.
>>>>>
>>>>> Of course this would mean more changes to net/smc/smc_core.c - but
>>>>> loopback-ism could omit these "boiler-plate" functions.
>>>>>
>>>>
>>>> Hi Gerd.
>>>>
>>>> Thank you for the thoughts! I agree that checks like 'if(smcd->ops->xxx)'
>>>> can avoid the device driver from implementing unsupported operations. But I
>>>> am afraid that which operations need to be defined as 'optional' may differ
>>>> from different device perspectives (e.g. for loopback-ism they are vlan-related
>>>> opts and signal_event). So I perfer to simply let the smc protocol assume
>>>> that all operations have been implemented, and let drivers to decide which
>>>> ones are unsupported in implementation. What do you think?
>>>>
>>>> Thanks!
>>>>
>>>
>>> I agree with Gerd, in my opinion it is better to document ops as
>>> optional and then allow their function pointers to be NULL and check
>>> for that. Acting like they are supported and then they turn out to be
>>> nops to me seems to contradict the principle of least surprises. I also
>>> think we can find a subset of mandatory ops without which SMC-D is
>>> impossible and then everything else should be optional.
>>
>> I see. If we all agree to classify smcd_ops into mandatory and optional ones,
>> I'll add a patch to mark the optional ops and check if they are implemented.
> 
> Keep in mind I don't speak for the SMC maintainers but that does sound
> reasonable to me.
> 

Hi Wenjia and Jan, do you have any comments on this and [1]? Thanks!

[1] https://lore.kernel.org/netdev/60b4aec0b4bf4474d651b653c86c280dafc4518a.camel@linux.ibm.com/

>>
>>>
>>> As a first guess I think the following options may be mandatory:
>>>
>>> * query_remote_gid()
>>> * register_dmb()/unregister_dmb()
>>> * move_data()
>>>     For this one could argue that either move_data() or
>>>     attach_dmb()/detach_dmb() is required though personally I would
>>>     prefer to always have move_data() as a fallback and simple API
>>> * supports_v2()
>>> * get_local_gid()
>>> * get_chid()
>>> * get_dev()
>> I agree with this classification. Just one point, maybe we can take
>> supports_v2() as an optional ops, like support_dmb_nocopy()? e.g. if
>> it is not implemented, we treat it as an ISMv1.
>>
>> Thanks!
> 
> Interpreting a NULL supports_v2() as not supporting v2 sounds
> reasonable to me.

On 09.04.24 03:44, Wen Gu wrote:
> 
> 
> On 2024/4/4 23:15, Niklas Schnelle wrote:
>> On Thu, 2024-04-04 at 21:12 +0800, Wen Gu wrote:
>>>
>>> On 2024/4/4 19:42, Niklas Schnelle wrote:
>>>> On Thu, 2024-04-04 at 17:32 +0800, Wen Gu wrote:
>>>>>
>>>>> On 2024/4/4 00:25, Gerd Bayer wrote:
>>>>>> On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
>>>>>>> This implements some operations that loopback-ism does not support
>>>>>>> currently:
>>>>>>>     - vlan operations, since there is no strong use-case for it.
>>>>>>>     - signal_event operations, since there is no event to be processed
>>>>>>> by the loopback-ism device.
>>>>>>
>>>>>> Hi Wen,
>>>>>>
>>>>>> I wonder if the these operations that are not supported by loopback-ism
>>>>>> should rather be marked "optional" in the struct smcd_ops, and the
>>>>>> calling code should call these only when they are implemented.
>>>>>>
>>>>>> Of course this would mean more changes to net/smc/smc_core.c - but
>>>>>> loopback-ism could omit these "boiler-plate" functions.
>>>>>>
>>>>>
>>>>> Hi Gerd.
>>>>>
>>>>> Thank you for the thoughts! I agree that checks like 'if(smcd->ops->xxx)'
>>>>> can avoid the device driver from implementing unsupported operations. But I
>>>>> am afraid that which operations need to be defined as 'optional' may differ
>>>>> from different device perspectives (e.g. for loopback-ism they are vlan-related
>>>>> opts and signal_event). So I perfer to simply let the smc protocol assume
>>>>> that all operations have been implemented, and let drivers to decide which
>>>>> ones are unsupported in implementation. What do you think?
>>>>>
>>>>> Thanks!
>>>>>
>>>>
>>>> I agree with Gerd, in my opinion it is better to document ops as
>>>> optional and then allow their function pointers to be NULL and check
>>>> for that. Acting like they are supported and then they turn out to be
>>>> nops to me seems to contradict the principle of least surprises. I also
>>>> think we can find a subset of mandatory ops without which SMC-D is
>>>> impossible and then everything else should be optional.
>>>
>>> I see. If we all agree to classify smcd_ops into mandatory and optional ones,
>>> I'll add a patch to mark the optional ops and check if they are implemented.
>>
>> Keep in mind I don't speak for the SMC maintainers but that does sound
>> reasonable to me.
>>
> 
> Hi Wenjia and Jan, do you have any comments on this and [1]? Thanks!
> 
> [1] https://lore.kernel.org/netdev/60b4aec0b4bf4474d651b653c86c280dafc4518a.camel@linux.ibm.com/
> 
>>>
>>>>
>>>> As a first guess I think the following options may be mandatory:
>>>>
>>>> * query_remote_gid()
>>>> * register_dmb()/unregister_dmb()
>>>> * move_data()
>>>>     For this one could argue that either move_data() or
>>>>     attach_dmb()/detach_dmb() is required though personally I would
>>>>     prefer to always have move_data() as a fallback and simple API
>>>> * supports_v2()
>>>> * get_local_gid()
>>>> * get_chid()
>>>> * get_dev()
>>> I agree with this classification. Just one point, maybe we can take
>>> supports_v2() as an optional ops, like support_dmb_nocopy()? e.g. if
>>> it is not implemented, we treat it as an ISMv1.
>>>
>>> Thanks!
>>
>> Interpreting a NULL supports_v2() as not supporting v2 sounds
>> reasonable to me.
> 

Let me add my thoughts to the discussion:
For the vlan operations and signal_event operations that loopback-ism does
not support:
I like the idea to set the ops to NULL and make sure the caller checks that
and can live with it. That is readable and efficient.

I don't think there is a need to discuss a strategy now, which ops could be
optional in the future. This is all inside the kernel. loopback-ism is even 
inside the smc module. Such comments in the code get outdated very easily.

I would propose to mark those as optional struct smcd_ops, where all callers can 
handle a NULL pointer and still be productive.
Future support of other devices for SMC-D can update that.

On 2024/4/11 19:12, Alexandra Winter wrote:
> 
> 
> On 09.04.24 03:44, Wen Gu wrote:
>>
>>
>> On 2024/4/4 23:15, Niklas Schnelle wrote:
>>> On Thu, 2024-04-04 at 21:12 +0800, Wen Gu wrote:
>>>>
>>>> On 2024/4/4 19:42, Niklas Schnelle wrote:
>>>>> On Thu, 2024-04-04 at 17:32 +0800, Wen Gu wrote:
>>>>>>
>>>>>> On 2024/4/4 00:25, Gerd Bayer wrote:
>>>>>>> On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
>>>>>>>> This implements some operations that loopback-ism does not support
>>>>>>>> currently:
>>>>>>>>      - vlan operations, since there is no strong use-case for it.
>>>>>>>>      - signal_event operations, since there is no event to be processed
>>>>>>>> by the loopback-ism device.
>>>>>>>
>>>>>>> Hi Wen,
>>>>>>>
>>>>>>> I wonder if the these operations that are not supported by loopback-ism
>>>>>>> should rather be marked "optional" in the struct smcd_ops, and the
>>>>>>> calling code should call these only when they are implemented.
>>>>>>>
>>>>>>> Of course this would mean more changes to net/smc/smc_core.c - but
>>>>>>> loopback-ism could omit these "boiler-plate" functions.
>>>>>>>
>>>>>>
>>>>>> Hi Gerd.
>>>>>>
>>>>>> Thank you for the thoughts! I agree that checks like 'if(smcd->ops->xxx)'
>>>>>> can avoid the device driver from implementing unsupported operations. But I
>>>>>> am afraid that which operations need to be defined as 'optional' may differ
>>>>>> from different device perspectives (e.g. for loopback-ism they are vlan-related
>>>>>> opts and signal_event). So I perfer to simply let the smc protocol assume
>>>>>> that all operations have been implemented, and let drivers to decide which
>>>>>> ones are unsupported in implementation. What do you think?
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>
>>>>> I agree with Gerd, in my opinion it is better to document ops as
>>>>> optional and then allow their function pointers to be NULL and check
>>>>> for that. Acting like they are supported and then they turn out to be
>>>>> nops to me seems to contradict the principle of least surprises. I also
>>>>> think we can find a subset of mandatory ops without which SMC-D is
>>>>> impossible and then everything else should be optional.
>>>>
>>>> I see. If we all agree to classify smcd_ops into mandatory and optional ones,
>>>> I'll add a patch to mark the optional ops and check if they are implemented.
>>>
>>> Keep in mind I don't speak for the SMC maintainers but that does sound
>>> reasonable to me.
>>>
>>
>> Hi Wenjia and Jan, do you have any comments on this and [1]? Thanks!
>>
>> [1] https://lore.kernel.org/netdev/60b4aec0b4bf4474d651b653c86c280dafc4518a.camel@linux.ibm.com/
>>
>>>>
>>>>>
>>>>> As a first guess I think the following options may be mandatory:
>>>>>
>>>>> * query_remote_gid()
>>>>> * register_dmb()/unregister_dmb()
>>>>> * move_data()
>>>>>      For this one could argue that either move_data() or
>>>>>      attach_dmb()/detach_dmb() is required though personally I would
>>>>>      prefer to always have move_data() as a fallback and simple API
>>>>> * supports_v2()
>>>>> * get_local_gid()
>>>>> * get_chid()
>>>>> * get_dev()
>>>> I agree with this classification. Just one point, maybe we can take
>>>> supports_v2() as an optional ops, like support_dmb_nocopy()? e.g. if
>>>> it is not implemented, we treat it as an ISMv1.
>>>>
>>>> Thanks!
>>>
>>> Interpreting a NULL supports_v2() as not supporting v2 sounds
>>> reasonable to me.
>>
> 
> Let me add my thoughts to the discussion:
> For the vlan operations and signal_event operations that loopback-ism does
> not support:
> I like the idea to set the ops to NULL and make sure the caller checks that
> and can live with it. That is readable and efficient.
> 
> I don't think there is a need to discuss a strategy now, which ops could be
> optional in the future. This is all inside the kernel. loopback-ism is even
> inside the smc module. Such comments in the code get outdated very easily.
> 
> I would propose to mark those as optional struct smcd_ops, where all callers can
> handle a NULL pointer and still be productive.
> Future support of other devices for SMC-D can update that.
> 
> 

Hi Sandy, just to confirm if I understand you correctly.

You are proposing that don't draw a conclusion about the classification now,
but supplementally mark which one become a optional operation in struct smcd_ops
during the introduction of new devices for SMC-D.

On 12.04.24 04:02, Wen Gu wrote:
> Hi Sandy, just to confirm if I understand you correctly.
> 
> You are proposing that don't draw a conclusion about the classification now,
> but supplementally mark which one become a optional operation in struct smcd_ops
> during the introduction of new devices for SMC-D.

Yes.

On 12.04.24 04:02, Wen Gu wrote:
> 
> 
> On 2024/4/11 19:12, Alexandra Winter wrote:
>>
>>
>> On 09.04.24 03:44, Wen Gu wrote:
>>>
>>>
>>> On 2024/4/4 23:15, Niklas Schnelle wrote:
>>>> On Thu, 2024-04-04 at 21:12 +0800, Wen Gu wrote:
>>>>>
>>>>> On 2024/4/4 19:42, Niklas Schnelle wrote:
>>>>>> On Thu, 2024-04-04 at 17:32 +0800, Wen Gu wrote:
>>>>>>>
>>>>>>> On 2024/4/4 00:25, Gerd Bayer wrote:
>>>>>>>> On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
>>>>>>>>> This implements some operations that loopback-ism does not support
>>>>>>>>> currently:
>>>>>>>>>      - vlan operations, since there is no strong use-case for it.
>>>>>>>>>      - signal_event operations, since there is no event to be 
>>>>>>>>> processed
>>>>>>>>> by the loopback-ism device.
>>>>>>>>
>>>>>>>> Hi Wen,
>>>>>>>>
>>>>>>>> I wonder if the these operations that are not supported by 
>>>>>>>> loopback-ism
>>>>>>>> should rather be marked "optional" in the struct smcd_ops, and the
>>>>>>>> calling code should call these only when they are implemented.
>>>>>>>>
>>>>>>>> Of course this would mean more changes to net/smc/smc_core.c - but
>>>>>>>> loopback-ism could omit these "boiler-plate" functions.
>>>>>>>>
>>>>>>>
>>>>>>> Hi Gerd.
>>>>>>>
>>>>>>> Thank you for the thoughts! I agree that checks like 
>>>>>>> 'if(smcd->ops->xxx)'
>>>>>>> can avoid the device driver from implementing unsupported 
>>>>>>> operations. But I
>>>>>>> am afraid that which operations need to be defined as 'optional' 
>>>>>>> may differ
>>>>>>> from different device perspectives (e.g. for loopback-ism they 
>>>>>>> are vlan-related
>>>>>>> opts and signal_event). So I perfer to simply let the smc 
>>>>>>> protocol assume
>>>>>>> that all operations have been implemented, and let drivers to 
>>>>>>> decide which
>>>>>>> ones are unsupported in implementation. What do you think?
>>>>>>>
>>>>>>> Thanks!
>>>>>>>
>>>>>>
>>>>>> I agree with Gerd, in my opinion it is better to document ops as
>>>>>> optional and then allow their function pointers to be NULL and check
>>>>>> for that. Acting like they are supported and then they turn out to be
>>>>>> nops to me seems to contradict the principle of least surprises. I 
>>>>>> also
>>>>>> think we can find a subset of mandatory ops without which SMC-D is
>>>>>> impossible and then everything else should be optional.
>>>>>
>>>>> I see. If we all agree to classify smcd_ops into mandatory and 
>>>>> optional ones,
>>>>> I'll add a patch to mark the optional ops and check if they are 
>>>>> implemented.
>>>>
>>>> Keep in mind I don't speak for the SMC maintainers but that does sound
>>>> reasonable to me.
>>>>
>>>
>>> Hi Wenjia and Jan, do you have any comments on this and [1]? Thanks!
>>>
>>> [1] 
>>> https://lore.kernel.org/netdev/60b4aec0b4bf4474d651b653c86c280dafc4518a.camel@linux.ibm.com/
>>>
>>>>>
>>>>>>
>>>>>> As a first guess I think the following options may be mandatory:
>>>>>>
>>>>>> * query_remote_gid()
>>>>>> * register_dmb()/unregister_dmb()
>>>>>> * move_data()
>>>>>>      For this one could argue that either move_data() or
>>>>>>      attach_dmb()/detach_dmb() is required though personally I would
>>>>>>      prefer to always have move_data() as a fallback and simple API
>>>>>> * supports_v2()
>>>>>> * get_local_gid()
>>>>>> * get_chid()
>>>>>> * get_dev()
>>>>> I agree with this classification. Just one point, maybe we can take
>>>>> supports_v2() as an optional ops, like support_dmb_nocopy()? e.g. if
>>>>> it is not implemented, we treat it as an ISMv1.
>>>>>
>>>>> Thanks!
>>>>
>>>> Interpreting a NULL supports_v2() as not supporting v2 sounds
>>>> reasonable to me.
>>>
>>
>> Let me add my thoughts to the discussion:
>> For the vlan operations and signal_event operations that loopback-ism 
>> does
>> not support:
>> I like the idea to set the ops to NULL and make sure the caller checks 
>> that
>> and can live with it. That is readable and efficient.
>>
>> I don't think there is a need to discuss a strategy now, which ops 
>> could be
>> optional in the future. This is all inside the kernel. loopback-ism is 
>> even
>> inside the smc module. Such comments in the code get outdated very 
>> easily.
>>
>> I would propose to mark those as optional struct smcd_ops, where all 
>> callers can
>> handle a NULL pointer and still be productive.
>> Future support of other devices for SMC-D can update that.
>>
>>
> 
> Hi Sandy, just to confirm if I understand you correctly.
> 
> You are proposing that don't draw a conclusion about the classification 
> now,
> but supplementally mark which one become a optional operation in struct 
> smcd_ops
> during the introduction of new devices for SMC-D.

@Sandy, could you please elaborate your proposal, or comfirm what Wen 
interpreted is what you mean?
If it is like what he said. IMO, I don't think it's necessary to dicuss 
further on which ops could be mandatory or optional. It's actually clear 
to me which are mandatory. And the classification should be much cleaner 
for our code. However, I agree that the classification is not really in 
the scope of this patches series. Especially if it is too expensive to 
rebuild it, we do need a seperate set of cleanup patches to do it. Thus, 
I'd like to let Wen take the decisions by ihmself. Any objections?

@All, if anyone has any strong opinion, I appreciate it if you could 
bring up your options as soon as possible. That would help us to 
accelerate the whole process.

Thanks,
Wenjia