[PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

[PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

[Souradeep Chowdhury]

This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
The notifier chain is used to check the role switch status of EUD. Since EUD can
function only in device mode, other modules trying to do role-switch on the same
port have to first check the EUD status by calling this notifier chain and based
on the status proceed or block their role-switching step. The modules can call
the notifier through the call eud_notifier_call_chain and pass their own
role switch state as the argument. This chain will also be able to handle the
scenario of multiple modules switching roles on the same port since this can
create a priority and ordering among them for conflict resolution.

Souradeep Chowdhury (3):
  usb: misc: Add the interface for notifier call for Embedded USB
    Debugger(EUD)
  usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver
  MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)

 MAINTAINERS                          |  1 +
 drivers/usb/misc/qcom_eud.c          | 52 ++++++++++++++++++++++++++--
 drivers/usb/misc/qcom_eud_notifier.h | 10 ++++++
 3 files changed, 61 insertions(+), 2 deletions(-)
 create mode 100644 drivers/usb/misc/qcom_eud_notifier.h

--
2.17.1

[PATCH V1 1/3] usb: misc: Add the interface for notifier call for Embedded USB Debugger(EUD)

[Souradeep Chowdhury]

Add the interface for notifier calls that can be made to check the role
switch status of EUD. An external module can import this file and use the
wrapper functions eud_register_notify to register a notifier to the EUD
notifier chain, eud_unregister_notify to unregister the notifier from the
chain, eud_notifier_call_chain to call the notifier chain to check the
status of role-switch. These interfaces are provided to handle the case
where multiple modules might go for a role-switch on the same port in which
case EUD will be given priority over the rest.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 drivers/usb/misc/qcom_eud_notifier.h | 10 ++++++++++
 1 file changed, 10 insertions(+)
 create mode 100644 drivers/usb/misc/qcom_eud_notifier.h

diff --git a/drivers/usb/misc/qcom_eud_notifier.h b/drivers/usb/misc/qcom_eud_notifier.h
new file mode 100644
index 000000000000..0cae42a5f6bf
--- /dev/null
+++ b/drivers/usb/misc/qcom_eud_notifier.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Released under the GPLv2 only.
+ */
+
+#include <linux/notifier.h>
+
+extern int eud_register_notify(struct notifier_block *nb);
+extern void eud_unregister_notify(struct notifier_block *nb);
+extern void eud_notifier_call_chain(unsigned long role_switch_state);
-- 
2.17.1

Re: [PATCH V1 1/3] usb: misc: Add the interface for notifier call for Embedded USB Debugger(EUD)

[Greg KH]

On Wed, Jul 12, 2023 at 01:52:38PM +0530, Souradeep Chowdhury wrote:
> Add the interface for notifier calls that can be made to check the role
> switch status of EUD. An external module can import this file and use the
> wrapper functions eud_register_notify to register a notifier to the EUD
> notifier chain, eud_unregister_notify to unregister the notifier from the
> chain, eud_notifier_call_chain to call the notifier chain to check the
> status of role-switch. These interfaces are provided to handle the case
> where multiple modules might go for a role-switch on the same port in which
> case EUD will be given priority over the rest.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  drivers/usb/misc/qcom_eud_notifier.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>  create mode 100644 drivers/usb/misc/qcom_eud_notifier.h
> 
> diff --git a/drivers/usb/misc/qcom_eud_notifier.h b/drivers/usb/misc/qcom_eud_notifier.h
> new file mode 100644
> index 000000000000..0cae42a5f6bf
> --- /dev/null
> +++ b/drivers/usb/misc/qcom_eud_notifier.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Released under the GPLv2 only.
> + */
> +
> +#include <linux/notifier.h>
> +
> +extern int eud_register_notify(struct notifier_block *nb);
> +extern void eud_unregister_notify(struct notifier_block *nb);
> +extern void eud_notifier_call_chain(unsigned long role_switch_state);
> -- 
> 2.17.1
> 

Don't add .h files that no one actually use.

[PATCH V1 2/3] usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver

[Souradeep Chowdhury]

Add the notifier call chain to EUD driver. The notifier call chain
is added to check the role-switch status of EUD. When multiple
modules are switching roles on the same port, they need to call this
notifier to check the role-switch status of EUD. If EUD is disabled,
then the modules can go for the role-switch, otherwise it needs to
be blocked. The notifier chain can be used to link multiple modules
switching roles on the same port and create a ordering, priority and
conflict resolution among them. The wrapper functions are defined here
which can be used to register a notifier block to the chain, deregister
it and also call the chain.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 drivers/usb/misc/qcom_eud.c | 52 +++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
index 7f371ea1248c..e6c97a2cf2df 100644
--- a/drivers/usb/misc/qcom_eud.c
+++ b/drivers/usb/misc/qcom_eud.c
@@ -11,10 +11,13 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/usb/role.h>
+#include "qcom_eud_notifier.h"
 
 #define EUD_REG_INT1_EN_MASK	0x0024
 #define EUD_REG_INT_STATUS_1	0x0044
@@ -22,14 +25,16 @@
 #define EUD_REG_VBUS_INT_CLR	0x0080
 #define EUD_REG_CSR_EUD_EN	0x1014
 #define EUD_REG_SW_ATTACH_DET	0x1018
-#define EUD_REG_EUD_EN2		0x0000
+#define EUD_REG_EUD_EN2        0x0000
 
 #define EUD_ENABLE		BIT(0)
-#define EUD_INT_PET_EUD		BIT(0)
+#define EUD_INT_PET_EUD	BIT(0)
 #define EUD_INT_VBUS		BIT(2)
 #define EUD_INT_SAFE_MODE	BIT(4)
 #define EUD_INT_ALL		(EUD_INT_VBUS | EUD_INT_SAFE_MODE)
 
+static RAW_NOTIFIER_HEAD(eud_nh);
+
 struct eud_chip {
 	struct device			*dev;
 	struct usb_role_switch		*role_sw;
@@ -41,6 +46,42 @@ struct eud_chip {
 	bool				usb_attached;
 };
 
+int eud_register_notify(struct notifier_block *nb)
+{
+	return raw_notifier_chain_register(&eud_nh, nb);
+}
+EXPORT_SYMBOL_GPL(eud_register_notify);
+
+void eud_unregister_notify(struct notifier_block *nb)
+{
+	raw_notifier_chain_unregister(&eud_nh, nb);
+}
+EXPORT_SYMBOL_GPL(eud_unregister_notify);
+
+void eud_notifier_call_chain(unsigned long role_switch_state)
+{
+	raw_notifier_call_chain(&eud_nh, role_switch_state, NULL);
+}
+EXPORT_SYMBOL_GPL(eud_notifier_call_chain);
+
+static int eud_vbus_spoof_attach_detach(struct notifier_block *nb, unsigned long event,
+					void *data)
+{
+	struct device_node *eud = of_find_compatible_node(NULL, NULL, "qcom,eud");
+	struct device *eud_device = bus_find_device_by_of_node(&platform_bus_type, eud);
+	struct eud_chip  *eud_data = dev_get_drvdata(eud_device);
+
+	if (eud_data->enabled  && event != USB_ROLE_DEVICE)
+		return NOTIFY_BAD;
+	else
+		return NOTIFY_OK;
+}
+
+static struct notifier_block eud_notifier = {
+	.notifier_call = eud_vbus_spoof_attach_detach,
+	.priority = 1,
+};
+
 static int enable_eud(struct eud_chip *priv)
 {
 	writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
@@ -196,6 +237,10 @@ static int eud_probe(struct platform_device *pdev)
 		return dev_err_probe(chip->dev, ret,
 				"failed to add role switch release action\n");
 
+	ret = eud_register_notify(&eud_notifier);
+	if (ret)
+		return dev_err_probe(chip->dev, ret, "failed to register notifier\n");
+
 	chip->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(chip->base))
 		return PTR_ERR(chip->base);
@@ -226,6 +271,9 @@ static void eud_remove(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, false);
 	disable_irq_wake(chip->irq);
+	eud_unregister_notify(&eud_notifier);
+
+	return 0;
 }
 
 static const struct of_device_id eud_dt_match[] = {
-- 
2.17.1

Re: [PATCH V1 2/3] usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver

[Bhupesh Sharma]

Hi Souradeep,

On Wed, 12 Jul 2023 at 13:58, Souradeep Chowdhury
<quic_schowdhu@quicinc.com> wrote:
>
> Add the notifier call chain to EUD driver. The notifier call chain
> is added to check the role-switch status of EUD. When multiple
> modules are switching roles on the same port, they need to call this
> notifier to check the role-switch status of EUD. If EUD is disabled,
> then the modules can go for the role-switch, otherwise it needs to
> be blocked. The notifier chain can be used to link multiple modules
> switching roles on the same port and create a ordering, priority and
> conflict resolution among them. The wrapper functions are defined here
> which can be used to register a notifier block to the chain, deregister
> it and also call the chain.
>
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  drivers/usb/misc/qcom_eud.c | 52 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 7f371ea1248c..e6c97a2cf2df 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -11,10 +11,13 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
>  #include <linux/usb/role.h>
> +#include "qcom_eud_notifier.h"
>
>  #define EUD_REG_INT1_EN_MASK   0x0024
>  #define EUD_REG_INT_STATUS_1   0x0044
> @@ -22,14 +25,16 @@
>  #define EUD_REG_VBUS_INT_CLR   0x0080
>  #define EUD_REG_CSR_EUD_EN     0x1014
>  #define EUD_REG_SW_ATTACH_DET  0x1018
> -#define EUD_REG_EUD_EN2                0x0000
> +#define EUD_REG_EUD_EN2        0x0000
>
>  #define EUD_ENABLE             BIT(0)
> -#define EUD_INT_PET_EUD                BIT(0)

These indentation issues are already addressed in my EUD patches.
Please rebase your patches on the same to reuse those.

> +#define EUD_INT_PET_EUD        BIT(0)
>  #define EUD_INT_VBUS           BIT(2)
>  #define EUD_INT_SAFE_MODE      BIT(4)
>  #define EUD_INT_ALL            (EUD_INT_VBUS | EUD_INT_SAFE_MODE)
>
> +static RAW_NOTIFIER_HEAD(eud_nh);
> +
>  struct eud_chip {
>         struct device                   *dev;
>         struct usb_role_switch          *role_sw;
> @@ -41,6 +46,42 @@ struct eud_chip {
>         bool                            usb_attached;
>  };
>
> +int eud_register_notify(struct notifier_block *nb)
> +{
> +       return raw_notifier_chain_register(&eud_nh, nb);
> +}
> +EXPORT_SYMBOL_GPL(eud_register_notify);
> +
> +void eud_unregister_notify(struct notifier_block *nb)
> +{
> +       raw_notifier_chain_unregister(&eud_nh, nb);
> +}
> +EXPORT_SYMBOL_GPL(eud_unregister_notify);
> +
> +void eud_notifier_call_chain(unsigned long role_switch_state)
> +{
> +       raw_notifier_call_chain(&eud_nh, role_switch_state, NULL);
> +}
> +EXPORT_SYMBOL_GPL(eud_notifier_call_chain);

Probably I missed it, but you have not provided any example users of
these APIs in the patchset or reference to another one which shows how
these APIs are used.

> +static int eud_vbus_spoof_attach_detach(struct notifier_block *nb, unsigned long event,
> +                                       void *data)
> +{
> +       struct device_node *eud = of_find_compatible_node(NULL, NULL, "qcom,eud");
> +       struct device *eud_device = bus_find_device_by_of_node(&platform_bus_type, eud);
> +       struct eud_chip  *eud_data = dev_get_drvdata(eud_device);
> +
> +       if (eud_data->enabled  && event != USB_ROLE_DEVICE)
> +               return NOTIFY_BAD;
> +       else
> +               return NOTIFY_OK;
> +}
> +
> +static struct notifier_block eud_notifier = {
> +       .notifier_call = eud_vbus_spoof_attach_detach,
> +       .priority = 1,

Why do you need a 'priority = 1' here, it can be 0 or even lower?

Thanks,
Bhupesh

> +};
> +
>  static int enable_eud(struct eud_chip *priv)
>  {
>         writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
> @@ -196,6 +237,10 @@ static int eud_probe(struct platform_device *pdev)
>                 return dev_err_probe(chip->dev, ret,
>                                 "failed to add role switch release action\n");
>
> +       ret = eud_register_notify(&eud_notifier);
> +       if (ret)
> +               return dev_err_probe(chip->dev, ret, "failed to register notifier\n");
> +
>         chip->base = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(chip->base))
>                 return PTR_ERR(chip->base);
> @@ -226,6 +271,9 @@ static void eud_remove(struct platform_device *pdev)
>
>         device_init_wakeup(&pdev->dev, false);
>         disable_irq_wake(chip->irq);
> +       eud_unregister_notify(&eud_notifier);
> +
> +       return 0;
>  }
>
>  static const struct of_device_id eud_dt_match[] = {
> --
> 2.17.1
>

Re: [PATCH V1 2/3] usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver

[Souradeep Chowdhury]

On 7/12/2023 11:52 PM, Bhupesh Sharma wrote:
> Hi Souradeep,
> 
> On Wed, 12 Jul 2023 at 13:58, Souradeep Chowdhury
> <quic_schowdhu@quicinc.com> wrote:
>>
>> Add the notifier call chain to EUD driver. The notifier call chain
>> is added to check the role-switch status of EUD. When multiple
>> modules are switching roles on the same port, they need to call this
>> notifier to check the role-switch status of EUD. If EUD is disabled,
>> then the modules can go for the role-switch, otherwise it needs to
>> be blocked. The notifier chain can be used to link multiple modules
>> switching roles on the same port and create a ordering, priority and
>> conflict resolution among them. The wrapper functions are defined here
>> which can be used to register a notifier block to the chain, deregister
>> it and also call the chain.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   drivers/usb/misc/qcom_eud.c | 52 +++++++++++++++++++++++++++++++++++--
>>   1 file changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
>> index 7f371ea1248c..e6c97a2cf2df 100644
>> --- a/drivers/usb/misc/qcom_eud.c
>> +++ b/drivers/usb/misc/qcom_eud.c
>> @@ -11,10 +11,13 @@
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>   #include <linux/sysfs.h>
>>   #include <linux/usb/role.h>
>> +#include "qcom_eud_notifier.h"
>>
>>   #define EUD_REG_INT1_EN_MASK   0x0024
>>   #define EUD_REG_INT_STATUS_1   0x0044
>> @@ -22,14 +25,16 @@
>>   #define EUD_REG_VBUS_INT_CLR   0x0080
>>   #define EUD_REG_CSR_EUD_EN     0x1014
>>   #define EUD_REG_SW_ATTACH_DET  0x1018
>> -#define EUD_REG_EUD_EN2                0x0000
>> +#define EUD_REG_EUD_EN2        0x0000
>>
>>   #define EUD_ENABLE             BIT(0)
>> -#define EUD_INT_PET_EUD                BIT(0)
> 
> These indentation issues are already addressed in my EUD patches.
> Please rebase your patches on the same to reuse those
Ack

> 
>> +#define EUD_INT_PET_EUD        BIT(0)
>>   #define EUD_INT_VBUS           BIT(2)
>>   #define EUD_INT_SAFE_MODE      BIT(4)
>>   #define EUD_INT_ALL            (EUD_INT_VBUS | EUD_INT_SAFE_MODE)
>>
>> +static RAW_NOTIFIER_HEAD(eud_nh);
>> +
>>   struct eud_chip {
>>          struct device                   *dev;
>>          struct usb_role_switch          *role_sw;
>> @@ -41,6 +46,42 @@ struct eud_chip {
>>          bool                            usb_attached;
>>   };
>>
>> +int eud_register_notify(struct notifier_block *nb)
>> +{
>> +       return raw_notifier_chain_register(&eud_nh, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(eud_register_notify);
>> +
>> +void eud_unregister_notify(struct notifier_block *nb)
>> +{
>> +       raw_notifier_chain_unregister(&eud_nh, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(eud_unregister_notify);
>> +
>> +void eud_notifier_call_chain(unsigned long role_switch_state)
>> +{
>> +       raw_notifier_call_chain(&eud_nh, role_switch_state, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(eud_notifier_call_chain);
> 
> Probably I missed it, but you have not provided any example users of
> these APIs in the patchset or reference to another one which shows how
> these APIs are used.

Ack, the usage for this will be posted in the next version.

> 
>> +static int eud_vbus_spoof_attach_detach(struct notifier_block *nb, unsigned long event,
>> +                                       void *data)
>> +{
>> +       struct device_node *eud = of_find_compatible_node(NULL, NULL, "qcom,eud");
>> +       struct device *eud_device = bus_find_device_by_of_node(&platform_bus_type, eud);
>> +       struct eud_chip  *eud_data = dev_get_drvdata(eud_device);
>> +
>> +       if (eud_data->enabled  && event != USB_ROLE_DEVICE)
>> +               return NOTIFY_BAD;
>> +       else
>> +               return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block eud_notifier = {
>> +       .notifier_call = eud_vbus_spoof_attach_detach,
>> +       .priority = 1,
> 
> Why do you need a 'priority = 1' here, it can be 0 or even lower?

Priority is 0 by default for a notifier block, since eud notifier
needs to be the first to be called in the chain to check the
role-switch status, I have kept the priority as 1 here.

> 
> Thanks,
> Bhupesh
> 
>> +};
>> +
>>   static int enable_eud(struct eud_chip *priv)
>>   {
>>          writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
>> @@ -196,6 +237,10 @@ static int eud_probe(struct platform_device *pdev)
>>                  return dev_err_probe(chip->dev, ret,
>>                                  "failed to add role switch release action\n");
>>
>> +       ret = eud_register_notify(&eud_notifier);
>> +       if (ret)
>> +               return dev_err_probe(chip->dev, ret, "failed to register notifier\n");
>> +
>>          chip->base = devm_platform_ioremap_resource(pdev, 0);
>>          if (IS_ERR(chip->base))
>>                  return PTR_ERR(chip->base);
>> @@ -226,6 +271,9 @@ static void eud_remove(struct platform_device *pdev)
>>
>>          device_init_wakeup(&pdev->dev, false);
>>          disable_irq_wake(chip->irq);
>> +       eud_unregister_notify(&eud_notifier);
>> +
>> +       return 0;
>>   }
>>
>>   static const struct of_device_id eud_dt_match[] = {
>> --
>> 2.17.1
>>

[PATCH V1 3/3] MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)

[Souradeep Chowdhury]

Add the entry for Embedded USB Debugger(EUD) header file which contains
interface definitions for the EUD notifier chain.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3be1bdfe8ecc..6d395cc6f45c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17288,6 +17288,7 @@ L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
 F:	Documentation/ABI/testing/sysfs-driver-eud
 F:	Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
+F:	drivers/usb/misc/qcom_eud_notifier.h
 F:	drivers/usb/misc/qcom_eud.c
 
 QCOM IPA DRIVER
-- 
2.17.1

Re: [PATCH V1 3/3] MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)

[Bhupesh Sharma]

On Wed, 12 Jul 2023 at 13:58, Souradeep Chowdhury
<quic_schowdhu@quicinc.com> wrote:
>
> Add the entry for Embedded USB Debugger(EUD) header file which contains
> interface definitions for the EUD notifier chain.
>
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3be1bdfe8ecc..6d395cc6f45c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17288,6 +17288,7 @@ L:      linux-arm-msm@vger.kernel.org
>  S:     Maintained
>  F:     Documentation/ABI/testing/sysfs-driver-eud
>  F:     Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> +F:     drivers/usb/misc/qcom_eud_notifier.h
>  F:     drivers/usb/misc/qcom_eud.c

You can simplify it to 'drivers/usb/misc/qcom_eud*' instead, for
avoiding repeatedly changing this when new files are added in future.

Thanks,
Bhupesh

Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

[Greg KH]

On Wed, Jul 12, 2023 at 01:52:37PM +0530, Souradeep Chowdhury wrote:
> This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
> The notifier chain is used to check the role switch status of EUD. Since EUD can
> function only in device mode, other modules trying to do role-switch on the same
> port have to first check the EUD status by calling this notifier chain and based
> on the status proceed or block their role-switching step. The modules can call
> the notifier through the call eud_notifier_call_chain and pass their own
> role switch state as the argument. This chain will also be able to handle the
> scenario of multiple modules switching roles on the same port since this can
> create a priority and ordering among them for conflict resolution.

You are adding a new api that no one is actually using, so why would we
accept this at all?

And how can we actually review it without any real users?

thanks,

greg k-h

Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

[Souradeep Chowdhury]

On 7/12/2023 10:10 PM, Greg KH wrote:
> On Wed, Jul 12, 2023 at 01:52:37PM +0530, Souradeep Chowdhury wrote:
>> This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
>> The notifier chain is used to check the role switch status of EUD. Since EUD can
>> function only in device mode, other modules trying to do role-switch on the same
>> port have to first check the EUD status by calling this notifier chain and based
>> on the status proceed or block their role-switching step. The modules can call
>> the notifier through the call eud_notifier_call_chain and pass their own
>> role switch state as the argument. This chain will also be able to handle the
>> scenario of multiple modules switching roles on the same port since this can
>> create a priority and ordering among them for conflict resolution.
> 
> You are adding a new api that no one is actually using, so why would we
> accept this at all?
> 
> And how can we actually review it without any real users?

Ack. The next version of this will be posted along with the usage
of these apis for ordering the role-switch.

> 
> thanks,
> 
> greg k-h

Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

[Greg KH]

On Thu, Jul 13, 2023 at 01:55:34PM +0530, Souradeep Chowdhury wrote:
> 
> 
> On 7/12/2023 10:10 PM, Greg KH wrote:
> > On Wed, Jul 12, 2023 at 01:52:37PM +0530, Souradeep Chowdhury wrote:
> > > This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
> > > The notifier chain is used to check the role switch status of EUD. Since EUD can
> > > function only in device mode, other modules trying to do role-switch on the same
> > > port have to first check the EUD status by calling this notifier chain and based
> > > on the status proceed or block their role-switching step. The modules can call
> > > the notifier through the call eud_notifier_call_chain and pass their own
> > > role switch state as the argument. This chain will also be able to handle the
> > > scenario of multiple modules switching roles on the same port since this can
> > > create a priority and ordering among them for conflict resolution.
> > 
> > You are adding a new api that no one is actually using, so why would we
> > accept this at all?
> > 
> > And how can we actually review it without any real users?
> 
> Ack. The next version of this will be posted along with the usage
> of these apis for ordering the role-switch.

Also note that I hate notifier call chains, so you better have a very
good reason for why they are required and can't be done in any other
normal way instead :)

good luck!

greg k-h

Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

[Konrad Dybcio]

On 12.07.2023 10:22, Souradeep Chowdhury wrote:
> This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
> The notifier chain is used to check the role switch status of EUD. Since EUD can
> function only in device mode, other modules trying to do role-switch on the same
> port have to first check the EUD status by calling this notifier chain and based
> on the status proceed or block their role-switching step. The modules can call
> the notifier through the call eud_notifier_call_chain and pass their own
> role switch state as the argument. This chain will also be able to handle the
> scenario of multiple modules switching roles on the same port since this can
> create a priority and ordering among them for conflict resolution.
> 
> Souradeep Chowdhury (3):
>   usb: misc: Add the interface for notifier call for Embedded USB
>     Debugger(EUD)
>   usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver
>   MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)
Please actually CC all maintainers, as present in the MAINTAINERS file..

Consider using b4:

https://b4.docs.kernel.org/en/latest/index.html

Konrad
> 
>  MAINTAINERS                          |  1 +
>  drivers/usb/misc/qcom_eud.c          | 52 ++++++++++++++++++++++++++--
>  drivers/usb/misc/qcom_eud_notifier.h | 10 ++++++
>  3 files changed, 61 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/usb/misc/qcom_eud_notifier.h
> 
> --
> 2.17.1
> 

Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

[Souradeep Chowdhury]

On 7/12/2023 10:13 PM, Konrad Dybcio wrote:
> 
> 
> On 12.07.2023 10:22, Souradeep Chowdhury wrote:
>> This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
>> The notifier chain is used to check the role switch status of EUD. Since EUD can
>> function only in device mode, other modules trying to do role-switch on the same
>> port have to first check the EUD status by calling this notifier chain and based
>> on the status proceed or block their role-switching step. The modules can call
>> the notifier through the call eud_notifier_call_chain and pass their own
>> role switch state as the argument. This chain will also be able to handle the
>> scenario of multiple modules switching roles on the same port since this can
>> create a priority and ordering among them for conflict resolution.
>>
>> Souradeep Chowdhury (3):
>>    usb: misc: Add the interface for notifier call for Embedded USB
>>      Debugger(EUD)
>>    usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver
>>    MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)
> Please actually CC all maintainers, as present in the MAINTAINERS file..
> 
> Consider using b4:
> 
> https://b4.docs.kernel.org/en/latest/index.html

Ack

> 
> Konrad
>>
>>   MAINTAINERS                          |  1 +
>>   drivers/usb/misc/qcom_eud.c          | 52 ++++++++++++++++++++++++++--
>>   drivers/usb/misc/qcom_eud_notifier.h | 10 ++++++
>>   3 files changed, 61 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/usb/misc/qcom_eud_notifier.h
>>
>> --
>> 2.17.1
>>