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

[Hans de Goede <hdegoede@redhat.com>]

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)
>