Hi, On 10/6/21 6:32 AM, lindsey.stanpoor@xxxxxxxxx wrote: > From: Cameron Nemo <cnemo@xxxxxxxxxxxx> > > 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@xxxxxxxxxx > > Signed-off-by: Cameron Nemo <cnemo@xxxxxxxxxxxx> 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) >
