[PATCH 1/1] usb: typec: altmodes/displayport: reorder dp_altmode_configured()

[PATCH 1/1] usb: typec: altmodes/displayport: reorder dp_altmode_configured()

[lindsey.stanpoor]

From: Cameron Nemo <cnemo@tutanota.com>

A recent commit [1] introduced an unintended behavioral change by
reordering certain function calls. The sysfs_notify call for
pin_assignment should only be invoked when the dp_altmode_notify call
returns 0, and in the dp->data.conf == 0 case.

[1] https://lore.kernel.org/r/20210817215201.795062-8-hdegoede@redhat.com

Signed-off-by: Cameron Nemo <cnemo@tutanota.com>
---
 drivers/usb/typec/altmodes/displayport.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index c1d8c23baa39..a15ae78066e3 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -154,10 +154,17 @@ static int dp_altmode_status_update(struct dp_altmode *dp)
 
 static int dp_altmode_configured(struct dp_altmode *dp)
 {
+	int ret;
+
 	sysfs_notify(&dp->alt->dev.kobj, "displayport", "configuration");
+
+	ret = dp_altmode_notify(dp);
+	if (ret || !dp->data.conf)
+		return ret;
+
 	sysfs_notify(&dp->alt->dev.kobj, "displayport", "pin_assignment");
 
-	return dp_altmode_notify(dp);
+	return 0;
 }
 
 static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf)
-- 
2.33.0

Re: [PATCH 1/1] usb: typec: altmodes/displayport: reorder dp_altmode_configured()

[Heikki Krogerus]

On Tue, Oct 05, 2021 at 09:32:57PM -0700, lindsey.stanpoor@gmail.com wrote:
> From: Cameron Nemo <cnemo@tutanota.com>
> 
> A recent commit [1] introduced an unintended behavioral change by
> reordering certain function calls. The sysfs_notify call for
> pin_assignment should only be invoked when the dp_altmode_notify call
> returns 0, and in the dp->data.conf == 0 case.
> 
> [1] https://lore.kernel.org/r/20210817215201.795062-8-hdegoede@redhat.com

You should refer the commit, not the mail. I think you could also use
the Fixes tag in this case, but then I guess Hans should pick this:

Fixes: fc27e04630e9 ("usb: typec: altmodes/displayport: Make dp_altmode_notify() more generic")

Either way, this is OK by me, so once you have cleaned the commit
message, please feel free to include my:
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> Signed-off-by: Cameron Nemo <cnemo@tutanota.com>
> ---
>  drivers/usb/typec/altmodes/displayport.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index c1d8c23baa39..a15ae78066e3 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -154,10 +154,17 @@ static int dp_altmode_status_update(struct dp_altmode *dp)
>  
>  static int dp_altmode_configured(struct dp_altmode *dp)
>  {
> +	int ret;
> +
>  	sysfs_notify(&dp->alt->dev.kobj, "displayport", "configuration");
> +
> +	ret = dp_altmode_notify(dp);
> +	if (ret || !dp->data.conf)
> +		return ret;
> +
>  	sysfs_notify(&dp->alt->dev.kobj, "displayport", "pin_assignment");
>  
> -	return dp_altmode_notify(dp);
> +	return 0;
>  }
>  
>  static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf)
> -- 
> 2.33.0

-- 
heikki

Re: [PATCH 1/1] usb: typec: altmodes/displayport: reorder dp_altmode_configured()

[Hans de Goede]

Hi,

On 10/6/21 6:32 AM, lindsey.stanpoor@gmail.com wrote:
> From: Cameron Nemo <cnemo@tutanota.com>
> 
> A recent commit [1] introduced an unintended behavioral change by
> reordering certain function calls. The sysfs_notify call for
> pin_assignment should only be invoked when the dp_altmode_notify call
> returns 0, and in the dp->data.conf == 0 case.
> 
> [1] https://lore.kernel.org/r/20210817215201.795062-8-hdegoede@redhat.com
> 
> Signed-off-by: Cameron Nemo <cnemo@tutanota.com>

You are right that my commit changed the behavior I should have added
something about that to the commit message, *but* I believe
that the new behavior is correct and should be kept.

Even if the dp_altmode_notify() fails, then reading from
the pin_assignment sysfs file will still show the new pin-assignment,
so the contents of the sysfs file has changed and thus the notify is
the correct thing to do independent of the dp_altmode_notify() 
return value.

Likewise if we have selected a pin_assignment ourselves and the
user tries to change this by writing to the pin_assignment sysfs
file, then we may get an async (so not signalled as an error
on the sysfs write syscall) CMDT_RSP_NAK to the new pin_assignment
at which point we set dp->data.conf = 0; and then call
dp_altmode_configured() and in this case again the
sysfs file "contents" has changed so we should do a notify.

More-over doing a syfs-notify when nothing has changed is really
not the end of the world, it is not like we are going to do this
100s of times per second.

IOW I believe that the new behavior although different is
correct (and the new code is also a lot more straight forward).

So NACK from me for this change.

Question, does this patch fix an actual problem which you were
seeing, or did you just notice this while reviewing the change ?

Regards,

Hans




> ---
>  drivers/usb/typec/altmodes/displayport.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index c1d8c23baa39..a15ae78066e3 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -154,10 +154,17 @@ static int dp_altmode_status_update(struct dp_altmode *dp)
>  
>  static int dp_altmode_configured(struct dp_altmode *dp)
>  {
> +	int ret;
> +
>  	sysfs_notify(&dp->alt->dev.kobj, "displayport", "configuration");
> +
> +	ret = dp_altmode_notify(dp);
> +	if (ret || !dp->data.conf)
> +		return ret;
> +
>  	sysfs_notify(&dp->alt->dev.kobj, "displayport", "pin_assignment");
>  
> -	return dp_altmode_notify(dp);
> +	return 0;
>  }
>  
>  static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf)
>