| Message ID | 20231108103226.1168500-4-arkadiusz.kubalewski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dpll: fix unordered unbind/bind registerer issues | expand |
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. >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. >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 >
>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 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 >>
On 09/11/2023 09:59, Kubalewski, Arkadiusz 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 > 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. > But why do you need other pins then, if FP0 doesn't exist? >> >>> 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.. > I just wonder how do you see the registration procedure? How can parent pin exist if it's not registered? I believe you cannot get it through DPLL API, then the only possible way is to create it within the same driver code, which can be simply re-arranged. Am I wrong here? > 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 >>>
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 >>>
>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >Sent: Thursday, November 9, 2023 11:56 AM >To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko > >On 09/11/2023 09:59, Kubalewski, Arkadiusz 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 >> 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. >> >But why do you need other pins then, if FP0 doesn't exist? > In general we don't need them at that point, but this is a corner case, where users for some reason decided to unbind PF 0, and I treat this state as temporary, where dpll/pins controllability is temporarily broken. The dpll at that point is not registered, all the direct pins are also not registered, thus not available to the users. When I do dump at that point there are still 3 pins present, one for each PF, although they are all zombies - no parents as their parent pins are not registered (as the other patch [1/3] prevents dump of pin parent if the parent is not registered). Maybe we can remove the REGISTERED mark for all the muxed pins, if all their parents have been unregistered, so they won't be visible to the user at all. Will try to POC that. >>> >>>> 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.. >> > >I just wonder how do you see the registration procedure? How can parent >pin exist if it's not registered? I believe you cannot get it through >DPLL API, then the only possible way is to create it within the same >driver code, which can be simply re-arranged. Am I wrong here? > By "parent exist" I mean the parent pin exist in the dpll subsystem (allocated on pins xa), but it doesn't mean it is available to the users, as it might not be registered with a dpll device. We have this 2 step init approach: 1. dpll_pin_get(..) -> allocate new pin or increase reference if exist 2.1. dpll_pin_register(..) -> register with a dpll device 2.2. dpll_pin_on_pin_register -> register with a parent pin Basically: - PF 0 does 1 & 2.1 for all the direct inputs, and steps: 1 & 2.2 for its recovery clock pin, - other PF's only do step 1 for the direct input pins (as they must get reference to those in order to register recovery clock pin with them), and steps: 1 & 2.2 for their recovery clock pin. Thank you! Arkadiusz >> 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 >>>> >
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, November 9, 2023 2:20 PM > >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? > Well, from my perspective FW design it is. AFAIR this single point of control is somehow related to HW design and security requirements back when it was designed.. Don't think this would be doable anytime soon (if doable at all). Thank you! Arkadiusz > >>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 >>>>
Thu, Nov 09, 2023 at 05:02:48PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>Sent: Thursday, November 9, 2023 11:56 AM >>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >> >>On 09/11/2023 09:59, Kubalewski, Arkadiusz 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 >>> 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. >>> >>But why do you need other pins then, if FP0 doesn't exist? >> > >In general we don't need them at that point, but this is a corner case, >where users for some reason decided to unbind PF 0, and I treat this state >as temporary, where dpll/pins controllability is temporarily broken. So resolve this broken situation internally in the driver, registering things only in case PF0 is present. Some simple notification infra would do. Don't drag this into the subsystem internals. > >The dpll at that point is not registered, all the direct pins are also >not registered, thus not available to the users. > >When I do dump at that point there are still 3 pins present, one for each >PF, although they are all zombies - no parents as their parent pins are not >registered (as the other patch [1/3] prevents dump of pin parent if the >parent is not registered). Maybe we can remove the REGISTERED mark for all >the muxed pins, if all their parents have been unregistered, so they won't >be visible to the user at all. Will try to POC that. > >>>> >>>>> 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.. >>> >> >>I just wonder how do you see the registration procedure? How can parent >>pin exist if it's not registered? I believe you cannot get it through >>DPLL API, then the only possible way is to create it within the same >>driver code, which can be simply re-arranged. Am I wrong here? >> > >By "parent exist" I mean the parent pin exist in the dpll subsystem >(allocated on pins xa), but it doesn't mean it is available to the users, >as it might not be registered with a dpll device. > >We have this 2 step init approach: >1. dpll_pin_get(..) -> allocate new pin or increase reference if exist >2.1. dpll_pin_register(..) -> register with a dpll device >2.2. dpll_pin_on_pin_register -> register with a parent pin > >Basically: >- PF 0 does 1 & 2.1 for all the direct inputs, and steps: 1 & 2.2 for its > recovery clock pin, >- other PF's only do step 1 for the direct input pins (as they must get > reference to those in order to register recovery clock pin with them), > and steps: 1 & 2.2 for their recovery clock pin. > > >Thank you! >Arkadiusz > >>> 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 >>>>> >> >
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, November 9, 2023 7:04 PM > >Thu, Nov 09, 2023 at 05:02:48PM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>Sent: Thursday, November 9, 2023 11:56 AM >>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >>> >>>On 09/11/2023 09:59, Kubalewski, Arkadiusz 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 >>>> 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. >>>> >>>But why do you need other pins then, if FP0 doesn't exist? >>> >> >>In general we don't need them at that point, but this is a corner case, >>where users for some reason decided to unbind PF 0, and I treat this state >>as temporary, where dpll/pins controllability is temporarily broken. > >So resolve this broken situation internally in the driver, registering >things only in case PF0 is present. Some simple notification infra would >do. Don't drag this into the subsystem internals. > Thanks for your feedback, but this is already wrong advice. Our HW/FW is designed in different way than yours, it doesn't mean it is wrong. As you might recall from our sync meetings, the dpll subsystem is to unify approaches and reduce the code in the drivers, where your advice is exactly opposite, suggested fix would require to implement extra synchronization of the dpll and pin registration state between driver instances, most probably with use of additional modules like aux-bus or something similar, which was from the very beginning something we tried to avoid. Only ice uses the infrastructure of muxed pins, and this is broken as it doesn't allow unbind the driver which have registered dpll and pins without crashing the kernel, so a fix is required in dpll subsystem, not in the driver. Thank you! Arkadiusz > >> >>The dpll at that point is not registered, all the direct pins are also >>not registered, thus not available to the users. >> >>When I do dump at that point there are still 3 pins present, one for each >>PF, although they are all zombies - no parents as their parent pins are >>not >>registered (as the other patch [1/3] prevents dump of pin parent if the >>parent is not registered). Maybe we can remove the REGISTERED mark for all >>the muxed pins, if all their parents have been unregistered, so they won't >>be visible to the user at all. Will try to POC that. >> >>>>> >>>>>> 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.. >>>> >>> >>>I just wonder how do you see the registration procedure? How can parent >>>pin exist if it's not registered? I believe you cannot get it through >>>DPLL API, then the only possible way is to create it within the same >>>driver code, which can be simply re-arranged. Am I wrong here? >>> >> >>By "parent exist" I mean the parent pin exist in the dpll subsystem >>(allocated on pins xa), but it doesn't mean it is available to the users, >>as it might not be registered with a dpll device. >> >>We have this 2 step init approach: >>1. dpll_pin_get(..) -> allocate new pin or increase reference if exist >>2.1. dpll_pin_register(..) -> register with a dpll device >>2.2. dpll_pin_on_pin_register -> register with a parent pin >> >>Basically: >>- PF 0 does 1 & 2.1 for all the direct inputs, and steps: 1 & 2.2 for its >> recovery clock pin, >>- other PF's only do step 1 for the direct input pins (as they must get >> reference to those in order to register recovery clock pin with them), >> and steps: 1 & 2.2 for their recovery clock pin. >> >> >>Thank you! >>Arkadiusz >> >>>> 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 >>>>>> >>> >>
Fri, Nov 10, 2023 at 12:21:11AM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Thursday, November 9, 2023 7:04 PM >> >>Thu, Nov 09, 2023 at 05:02:48PM CET, arkadiusz.kubalewski@intel.com wrote: >>>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>Sent: Thursday, November 9, 2023 11:56 AM >>>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >>>> >>>>On 09/11/2023 09:59, Kubalewski, Arkadiusz 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 >>>>> 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. >>>>> >>>>But why do you need other pins then, if FP0 doesn't exist? >>>> >>> >>>In general we don't need them at that point, but this is a corner case, >>>where users for some reason decided to unbind PF 0, and I treat this state >>>as temporary, where dpll/pins controllability is temporarily broken. >> >>So resolve this broken situation internally in the driver, registering >>things only in case PF0 is present. Some simple notification infra would >>do. Don't drag this into the subsystem internals. >> > >Thanks for your feedback, but this is already wrong advice. > >Our HW/FW is designed in different way than yours, it doesn't mean it is wrong. >As you might recall from our sync meetings, the dpll subsystem is to unify >approaches and reduce the code in the drivers, where your advice is exactly No. Your driver knows when what objects are valid or not. Of a pin of PF1 is not valid because the "master" PF0 is gone, it is responsibility of your driver to resolve that. Don't bring this internal dependencies to the dpll core please, does not make any sense to do so. Thanks! >opposite, suggested fix would require to implement extra synchronization of the >dpll and pin registration state between driver instances, most probably with >use of additional modules like aux-bus or something similar, which was from the >very beginning something we tried to avoid. >Only ice uses the infrastructure of muxed pins, and this is broken as it >doesn't allow unbind the driver which have registered dpll and pins without >crashing the kernel, so a fix is required in dpll subsystem, not in the driver. > >Thank you! >Arkadiusz > >> >>> >>>The dpll at that point is not registered, all the direct pins are also >>>not registered, thus not available to the users. >>> >>>When I do dump at that point there are still 3 pins present, one for each >>>PF, although they are all zombies - no parents as their parent pins are >>>not >>>registered (as the other patch [1/3] prevents dump of pin parent if the >>>parent is not registered). Maybe we can remove the REGISTERED mark for all >>>the muxed pins, if all their parents have been unregistered, so they won't >>>be visible to the user at all. Will try to POC that. >>> >>>>>> >>>>>>> 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.. >>>>> >>>> >>>>I just wonder how do you see the registration procedure? How can parent >>>>pin exist if it's not registered? I believe you cannot get it through >>>>DPLL API, then the only possible way is to create it within the same >>>>driver code, which can be simply re-arranged. Am I wrong here? >>>> >>> >>>By "parent exist" I mean the parent pin exist in the dpll subsystem >>>(allocated on pins xa), but it doesn't mean it is available to the users, >>>as it might not be registered with a dpll device. >>> >>>We have this 2 step init approach: >>>1. dpll_pin_get(..) -> allocate new pin or increase reference if exist >>>2.1. dpll_pin_register(..) -> register with a dpll device >>>2.2. dpll_pin_on_pin_register -> register with a parent pin >>> >>>Basically: >>>- PF 0 does 1 & 2.1 for all the direct inputs, and steps: 1 & 2.2 for its >>> recovery clock pin, >>>- other PF's only do step 1 for the direct input pins (as they must get >>> reference to those in order to register recovery clock pin with them), >>> and steps: 1 & 2.2 for their recovery clock pin. >>> >>> >>>Thank you! >>>Arkadiusz >>> >>>>> 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 >>>>>>> >>>> >>>
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Friday, November 10, 2023 7:48 AM > >Fri, Nov 10, 2023 at 12:21:11AM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Thursday, November 9, 2023 7:04 PM >>> >>>Thu, Nov 09, 2023 at 05:02:48PM CET, arkadiusz.kubalewski@intel.com >>>wrote: >>>>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>>Sent: Thursday, November 9, 2023 11:56 AM >>>>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >>>>> >>>>>On 09/11/2023 09:59, Kubalewski, Arkadiusz 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 >>>>>> 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. >>>>>> >>>>>But why do you need other pins then, if FP0 doesn't exist? >>>>> >>>> >>>>In general we don't need them at that point, but this is a corner case, >>>>where users for some reason decided to unbind PF 0, and I treat this >>>>state >>>>as temporary, where dpll/pins controllability is temporarily broken. >>> >>>So resolve this broken situation internally in the driver, registering >>>things only in case PF0 is present. Some simple notification infra would >>>do. Don't drag this into the subsystem internals. >>> >> >>Thanks for your feedback, but this is already wrong advice. >> >>Our HW/FW is designed in different way than yours, it doesn't mean it is >>wrong. >>As you might recall from our sync meetings, the dpll subsystem is to unify >>approaches and reduce the code in the drivers, where your advice is >>exactly > >No. Your driver knows when what objects are valid or not. Of a pin of >PF1 is not valid because the "master" PF0 is gone, it is responsibility >of your driver to resolve that. Don't bring this internal dependencies >to the dpll core please, does not make any sense to do so. Thanks! > No, a driver doesn't know it, those are separated instances, and you already suggested to implement special notification bus in the driver. This is not needed and prone for another errors. The dpll subsystem is here to make driver life easier. Thank you! Arkadiusz > >>opposite, suggested fix would require to implement extra synchronization >>of the >>dpll and pin registration state between driver instances, most probably >>with >>use of additional modules like aux-bus or something similar, which was >>from the >>very beginning something we tried to avoid. >>Only ice uses the infrastructure of muxed pins, and this is broken as it >>doesn't allow unbind the driver which have registered dpll and pins >>without >>crashing the kernel, so a fix is required in dpll subsystem, not in the >>driver. >> >>Thank you! >>Arkadiusz >> >>> >>>> >>>>The dpll at that point is not registered, all the direct pins are also >>>>not registered, thus not available to the users. >>>> >>>>When I do dump at that point there are still 3 pins present, one for >>>>each >>>>PF, although they are all zombies - no parents as their parent pins are >>>>not >>>>registered (as the other patch [1/3] prevents dump of pin parent if the >>>>parent is not registered). Maybe we can remove the REGISTERED mark for >>>>all >>>>the muxed pins, if all their parents have been unregistered, so they >>>>won't >>>>be visible to the user at all. Will try to POC that. >>>> >>>>>>> >>>>>>>> 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.. >>>>>> >>>>> >>>>>I just wonder how do you see the registration procedure? How can parent >>>>>pin exist if it's not registered? I believe you cannot get it through >>>>>DPLL API, then the only possible way is to create it within the same >>>>>driver code, which can be simply re-arranged. Am I wrong here? >>>>> >>>> >>>>By "parent exist" I mean the parent pin exist in the dpll subsystem >>>>(allocated on pins xa), but it doesn't mean it is available to the >>>>users, >>>>as it might not be registered with a dpll device. >>>> >>>>We have this 2 step init approach: >>>>1. dpll_pin_get(..) -> allocate new pin or increase reference if exist >>>>2.1. dpll_pin_register(..) -> register with a dpll device >>>>2.2. dpll_pin_on_pin_register -> register with a parent pin >>>> >>>>Basically: >>>>- PF 0 does 1 & 2.1 for all the direct inputs, and steps: 1 & 2.2 for >>>>its >>>> recovery clock pin, >>>>- other PF's only do step 1 for the direct input pins (as they must get >>>> reference to those in order to register recovery clock pin with them), >>>> and steps: 1 & 2.2 for their recovery clock pin. >>>> >>>> >>>>Thank you! >>>>Arkadiusz >>>> >>>>>> 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 >>>>>>>> >>>>> >>>>
Fri, Nov 10, 2023 at 09:50:34AM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Friday, November 10, 2023 7:48 AM >> >>Fri, Nov 10, 2023 at 12:21:11AM CET, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Thursday, November 9, 2023 7:04 PM >>>> >>>>Thu, Nov 09, 2023 at 05:02:48PM CET, arkadiusz.kubalewski@intel.com >>>>wrote: >>>>>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>>>Sent: Thursday, November 9, 2023 11:56 AM >>>>>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >>>>>> >>>>>>On 09/11/2023 09:59, Kubalewski, Arkadiusz 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 >>>>>>> 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. >>>>>>> >>>>>>But why do you need other pins then, if FP0 doesn't exist? >>>>>> >>>>> >>>>>In general we don't need them at that point, but this is a corner case, >>>>>where users for some reason decided to unbind PF 0, and I treat this >>>>>state >>>>>as temporary, where dpll/pins controllability is temporarily broken. >>>> >>>>So resolve this broken situation internally in the driver, registering >>>>things only in case PF0 is present. Some simple notification infra would >>>>do. Don't drag this into the subsystem internals. >>>> >>> >>>Thanks for your feedback, but this is already wrong advice. >>> >>>Our HW/FW is designed in different way than yours, it doesn't mean it is >>>wrong. >>>As you might recall from our sync meetings, the dpll subsystem is to unify >>>approaches and reduce the code in the drivers, where your advice is >>>exactly >> >>No. Your driver knows when what objects are valid or not. Of a pin of >>PF1 is not valid because the "master" PF0 is gone, it is responsibility >>of your driver to resolve that. Don't bring this internal dependencies >>to the dpll core please, does not make any sense to do so. Thanks! >> > >No, a driver doesn't know it, those are separated instances, and you already >suggested to implement special notification bus in the driver. >This is not needed and prone for another errors. The dpll subsystem is here to >make driver life easier. See the other thread for my reply. > >Thank you! >Arkadiusz > >> >>>opposite, suggested fix would require to implement extra synchronization >>>of the >>>dpll and pin registration state between driver instances, most probably >>>with >>>use of additional modules like aux-bus or something similar, which was >>>from the >>>very beginning something we tried to avoid. >>>Only ice uses the infrastructure of muxed pins, and this is broken as it >>>doesn't allow unbind the driver which have registered dpll and pins >>>without >>>crashing the kernel, so a fix is required in dpll subsystem, not in the >>>driver. >>> >>>Thank you! >>>Arkadiusz >>> >>>> >>>>> >>>>>The dpll at that point is not registered, all the direct pins are also >>>>>not registered, thus not available to the users. >>>>> >>>>>When I do dump at that point there are still 3 pins present, one for >>>>>each >>>>>PF, although they are all zombies - no parents as their parent pins are >>>>>not >>>>>registered (as the other patch [1/3] prevents dump of pin parent if the >>>>>parent is not registered). Maybe we can remove the REGISTERED mark for >>>>>all >>>>>the muxed pins, if all their parents have been unregistered, so they >>>>>won't >>>>>be visible to the user at all. Will try to POC that. >>>>> >>>>>>>> >>>>>>>>> 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.. >>>>>>> >>>>>> >>>>>>I just wonder how do you see the registration procedure? How can parent >>>>>>pin exist if it's not registered? I believe you cannot get it through >>>>>>DPLL API, then the only possible way is to create it within the same >>>>>>driver code, which can be simply re-arranged. Am I wrong here? >>>>>> >>>>> >>>>>By "parent exist" I mean the parent pin exist in the dpll subsystem >>>>>(allocated on pins xa), but it doesn't mean it is available to the >>>>>users, >>>>>as it might not be registered with a dpll device. >>>>> >>>>>We have this 2 step init approach: >>>>>1. dpll_pin_get(..) -> allocate new pin or increase reference if exist >>>>>2.1. dpll_pin_register(..) -> register with a dpll device >>>>>2.2. dpll_pin_on_pin_register -> register with a parent pin >>>>> >>>>>Basically: >>>>>- PF 0 does 1 & 2.1 for all the direct inputs, and steps: 1 & 2.2 for >>>>>its >>>>> recovery clock pin, >>>>>- other PF's only do step 1 for the direct input pins (as they must get >>>>> reference to those in order to register recovery clock pin with them), >>>>> and steps: 1 & 2.2 for their recovery clock pin. >>>>> >>>>> >>>>>Thank you! >>>>>Arkadiusz >>>>> >>>>>>> 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 >>>>>>>>> >>>>>> >>>>>
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Friday, November 10, 2023 11:07 AM > >Fri, Nov 10, 2023 at 09:50:34AM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Friday, November 10, 2023 7:48 AM >>> >>>Fri, Nov 10, 2023 at 12:21:11AM CET, arkadiusz.kubalewski@intel.com >>>wrote: >>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>Sent: Thursday, November 9, 2023 7:04 PM >>>>> >>>>>Thu, Nov 09, 2023 at 05:02:48PM CET, arkadiusz.kubalewski@intel.com >>>>>wrote: >>>>>>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>>>>Sent: Thursday, November 9, 2023 11:56 AM >>>>>>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri >>>>>>>Pirko >>>>>>> >>>>>>>On 09/11/2023 09:59, Kubalewski, Arkadiusz 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 >>>>>>>> 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. >>>>>>>> >>>>>>>But why do you need other pins then, if FP0 doesn't exist? >>>>>>> >>>>>> >>>>>>In general we don't need them at that point, but this is a corner >>>>>>case, >>>>>>where users for some reason decided to unbind PF 0, and I treat this >>>>>>state >>>>>>as temporary, where dpll/pins controllability is temporarily broken. >>>>> >>>>>So resolve this broken situation internally in the driver, registering >>>>>things only in case PF0 is present. Some simple notification infra >>>>>would >>>>>do. Don't drag this into the subsystem internals. >>>>> >>>> >>>>Thanks for your feedback, but this is already wrong advice. >>>> >>>>Our HW/FW is designed in different way than yours, it doesn't mean it is >>>>wrong. >>>>As you might recall from our sync meetings, the dpll subsystem is to >>>>unify >>>>approaches and reduce the code in the drivers, where your advice is >>>>exactly >>> >>>No. Your driver knows when what objects are valid or not. Of a pin of >>>PF1 is not valid because the "master" PF0 is gone, it is responsibility >>>of your driver to resolve that. Don't bring this internal dependencies >>>to the dpll core please, does not make any sense to do so. Thanks! >>> >> >>No, a driver doesn't know it, those are separated instances, and you >>already >>suggested to implement special notification bus in the driver. >>This is not needed and prone for another errors. The dpll subsystem is >>here to >>make driver life easier. > >See the other thread for my reply. > Ok, will do. Thank you! Arkadiusz >> >>Thank you! >>Arkadiusz >> >>> >>>>opposite, suggested fix would require to implement extra synchronization >>>>of the >>>>dpll and pin registration state between driver instances, most probably >>>>with >>>>use of additional modules like aux-bus or something similar, which was >>>>from the >>>>very beginning something we tried to avoid. >>>>Only ice uses the infrastructure of muxed pins, and this is broken as it >>>>doesn't allow unbind the driver which have registered dpll and pins >>>>without >>>>crashing the kernel, so a fix is required in dpll subsystem, not in the >>>>driver. >>>> >>>>Thank you! >>>>Arkadiusz >>>> >>>>> >>>>>> >>>>>>The dpll at that point is not registered, all the direct pins are also >>>>>>not registered, thus not available to the users. >>>>>> >>>>>>When I do dump at that point there are still 3 pins present, one for >>>>>>each >>>>>>PF, although they are all zombies - no parents as their parent pins >>>>>>are >>>>>>not >>>>>>registered (as the other patch [1/3] prevents dump of pin parent if >>>>>>the >>>>>>parent is not registered). Maybe we can remove the REGISTERED mark for >>>>>>all >>>>>>the muxed pins, if all their parents have been unregistered, so they >>>>>>won't >>>>>>be visible to the user at all. Will try to POC that. >>>>>> >>>>>>>>> >>>>>>>>>> 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.. >>>>>>>> >>>>>>> >>>>>>>I just wonder how do you see the registration procedure? How can >>>>>>>parent >>>>>>>pin exist if it's not registered? I believe you cannot get it through >>>>>>>DPLL API, then the only possible way is to create it within the same >>>>>>>driver code, which can be simply re-arranged. Am I wrong here? >>>>>>> >>>>>> >>>>>>By "parent exist" I mean the parent pin exist in the dpll subsystem >>>>>>(allocated on pins xa), but it doesn't mean it is available to the >>>>>>users, >>>>>>as it might not be registered with a dpll device. >>>>>> >>>>>>We have this 2 step init approach: >>>>>>1. dpll_pin_get(..) -> allocate new pin or increase reference if exist >>>>>>2.1. dpll_pin_register(..) -> register with a dpll device >>>>>>2.2. dpll_pin_on_pin_register -> register with a parent pin >>>>>> >>>>>>Basically: >>>>>>- PF 0 does 1 & 2.1 for all the direct inputs, and steps: 1 & 2.2 for >>>>>>its >>>>>> recovery clock pin, >>>>>>- other PF's only do step 1 for the direct input pins (as they must >>>>>>get >>>>>> reference to those in order to register recovery clock pin with >>>>>>them), >>>>>> and steps: 1 & 2.2 for their recovery clock pin. >>>>>> >>>>>> >>>>>>Thank you! >>>>>>Arkadiusz >>>>>> >>>>>>>> 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 >>>>>>>>>> >>>>>>> >>>>>>
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);
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. 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 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(-)