Re: [PATCH net 3/3] dpll: fix register pin with unregistered parent pin

[Jiri Pirko <jiri@resnulli.us>]

Thu, Nov 09, 2023 at 10:59:04AM CET, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Wednesday, November 8, 2023 4:08 PM
>>
>>Wed, Nov 08, 2023 at 11:32:26AM CET, arkadiusz.kubalewski@intel.com wrote:
>>>In case of multiple kernel module instances using the same dpll device:
>>>if only one registers dpll device, then only that one can register
>>
>>They why you don't register in multiple instances? See mlx5 for a
>>reference.
>>
>
>Every registration requires ops, but for our case only PF0 is able to

What makes PF0 so special? Smell like broken FW design... Care to fix
it?


>control dpll pins and device, thus only this can provide ops.
>Basically without PF0, dpll is not able to be controlled, as well
>as directly connected pins.
>
>>
>>>directly connected pins with a dpll device. If unregistered parent
>>>determines if the muxed pin can be register with it or not, it forces
>>>serialized driver load order - first the driver instance which
>>>registers the direct pins needs to be loaded, then the other instances
>>>could register muxed type pins.
>>>
>>>Allow registration of a pin with a parent even if the parent was not
>>>yet registered, thus allow ability for unserialized driver instance
>>
>>Weird.
>>
>
>Yeah, this is issue only for MUX/parent pin part, couldn't find better
>way, but it doesn't seem to break things around..
>
>Thank you!
>Arkadiusz
>
>>
>>>load order.
>>>Do not WARN_ON notification for unregistered pin, which can be invoked
>>>for described case, instead just return error.
>>>
>>>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base
>>>functions")
>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>---
>>> drivers/dpll/dpll_core.c    | 4 ----
>>> drivers/dpll/dpll_netlink.c | 2 +-
>>> 2 files changed, 1 insertion(+), 5 deletions(-)
>>>
>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c index
>>>4077b562ba3b..ae884b92d68c 100644
>>>--- a/drivers/dpll/dpll_core.c
>>>+++ b/drivers/dpll/dpll_core.c
>>>@@ -28,8 +28,6 @@ static u32 dpll_xa_id;
>>> 	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>> #define ASSERT_DPLL_NOT_REGISTERED(d)	\
>>> 	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>>-#define ASSERT_PIN_REGISTERED(p)	\
>>>-	WARN_ON_ONCE(!xa_get_mark(&dpll_pin_xa, (p)->id, DPLL_REGISTERED))
>>>
>>> struct dpll_device_registration {
>>> 	struct list_head list;
>>>@@ -641,8 +639,6 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent,
>>struct dpll_pin *pin,
>>> 	    WARN_ON(!ops->state_on_pin_get) ||
>>> 	    WARN_ON(!ops->direction_get))
>>> 		return -EINVAL;
>>>-	if (ASSERT_PIN_REGISTERED(parent))
>>>-		return -EINVAL;
>>>
>>> 	mutex_lock(&dpll_lock);
>>> 	ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv); diff
>>>--git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c index
>>>963bbbbe6660..ff430f43304f 100644
>>>--- a/drivers/dpll/dpll_netlink.c
>>>+++ b/drivers/dpll/dpll_netlink.c
>>>@@ -558,7 +558,7 @@ dpll_pin_event_send(enum dpll_cmd event, struct
>>dpll_pin *pin)
>>> 	int ret = -ENOMEM;
>>> 	void *hdr;
>>>
>>>-	if (WARN_ON(!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED)))
>>>+	if (!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED))
>>> 		return -ENODEV;
>>>
>>> 	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>>>--
>>>2.38.1
>>>