RE: [PATCH net v3 2/3] dpll: fix register pin with unregistered parent pin

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>Sent: Tuesday, January 16, 2024 8:42 AM
>
>Mon, Jan 15, 2024 at 09:52:40AM CET, arkadiusz.kubalewski@xxxxxxxxx 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
>>directly connected pins with a dpll device. When unregistered parent is
>>responsible for determining if the muxed pin can be registered with it
>>or not, the drivers need to be loaded in serialized order to work
>>correctly - 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
>>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")
>>Reviewed-by: Jan Glaza <jan.glaza@xxxxxxxxx>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>>---
>>v3:
>>- allow register with non registered parent dpll device for consistency
>>
>> drivers/dpll/dpll_core.c    | 6 ------
>> drivers/dpll/dpll_netlink.c | 2 +-
>> 2 files changed, 1 insertion(+), 7 deletions(-)
>>
>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>index fbac32af78b7..69005d8489d3 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))
>
>Also, for consistency, this could be called from
>__dpll_pin_unregister(). But that is net-next material.
>
>
>Reviewed-by: Jiri Pirko <jiri@xxxxxxxxxx>
>

Thank you!
Arkadiusz

>
>
>>
>> struct dpll_device_registration {
>> 	struct list_head list;
>>@@ -613,8 +611,6 @@ dpll_pin_register(struct dpll_device *dpll, struct
>dpll_pin *pin,
>> 	    WARN_ON(!ops->state_on_dpll_get) ||
>> 	    WARN_ON(!ops->direction_get))
>> 		return -EINVAL;
>>-	if (ASSERT_DPLL_REGISTERED(dpll))
>>-		return -EINVAL;
>>
>> 	mutex_lock(&dpll_lock);
>> 	if (WARN_ON(!(dpll->module == pin->module &&
>>@@ -692,8 +688,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 4c64611d32ac..108c002537e6 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -551,7 +551,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
>>