Hi Dan,
On Wed, Jan 31, 2024 at 09:59:43AM +0300, Dan Carpenter wrote:
> Hello Christian A. Ehrhardt,
>
> The patch f3be347ea42d: "usb: ucsi_acpi: Quirk to ack a connector
> change ack cmd" from Jan 21, 2024 (linux-next), leads to the
> following Smatch static checker warning:
>
> drivers/usb/typec/ucsi/ucsi_acpi.c:174 ucsi_dell_sync_write()
> warn: missing error code? 'ret'
>
> drivers/usb/typec/ucsi/ucsi_acpi.c
> 138 static int
> 139 ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,
> 140 const void *val, size_t val_len)
> 141 {
> 142 struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> 143 u64 cmd = *(u64 *)val, ack = 0;
> 144 int ret;
> 145
> 146 if (UCSI_COMMAND(cmd) == UCSI_ACK_CC_CI &&
> 147 cmd & UCSI_ACK_CONNECTOR_CHANGE)
> 148 ack = UCSI_ACK_CC_CI | UCSI_ACK_COMMAND_COMPLETE;
> 149
> 150 ret = ucsi_acpi_sync_write(ucsi, offset, val, val_len);
> 151 if (ret != 0)
> 152 return ret;
> 153 if (ack == 0)
> 154 return ret;
>
> This would be better as "return 0;"
While it is technically true that this could be written as "return 0;"
it was written in this way intentionally. The ucsi_dell_sync_write
quirk function is a wrapper around this call to ucsi_acpi_sync_write.
These if-statements handle the cases where nothing needs to be done
after that call and we just return with whatever ucsi_acpi_sync_write
returned.
> 155
> 156 if (!ua->dell_quirk_probed) {
> 157 ua->dell_quirk_probed = true;
> 158
> 159 cmd = UCSI_GET_CAPABILITY;
> 160 ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd,
> 161 sizeof(cmd));
> 162 if (ret == 0)
> 163 return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL,
> 164 &ack, sizeof(ack));
> 165 if (ret != -ETIMEDOUT)
> 166 return ret;
> 167
> 168 ua->dell_quirk_active = true;
>
> Here we set ->dell_quirk_active to true;
>
> 169 dev_err(ua->dev, "Firmware bug: Additional ACK required after ACKing a connector change.\n");
> 170 dev_err(ua->dev, "Firmware bug: Enabling workaround\n");
> 171 }
> 172
> 173 if (!ua->dell_quirk_active)
> --> 174 return ret;
>
> So this is basically an else statement and ret is zero. Smatch thinks
> it should be an error code but I think it's intentional. Why reverse
> the if statement, return a literal, and pull the code in a tab. You
> could delete the ua->dell_quirk_active variable. Or is the concern
> here that we'll be probing two of these quirky things concurrently?
> It's a bit strange and probably needs some cleanup or comments.
>
> if (ua->dell_quirk_probed)
> return 0;
This is not correct. ->dell_quirk_active may have been set in a
previous run and that's the whole point of the probe: Setting
->dell_quirk_active to the correct value for future calls to this
function.
> ua->dell_quirk_probed = true;
>
> cmd = UCSI_GET_CAPABILITY;
> ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
> ...
So this code should only be executed once.
>
> 175
> 176 return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &ack, sizeof(ack));
> 177 }
This additional call is the actual quirk. It should not be neccessary
according to the spec but if the probe determines that it is required
it must be done each time an ack for an async event is sent. If the
quirk is not neccessary we just return whatever the above call to
ucsi_acpi_sync_write() returned.
So in summary, I don't think there's a bug here.
However, it is possible to silence smatch with the diff below
(or with the second hunk alone) and I'd be ok with this. I can
give this a try on the real hardware and send a patch later this
week.
regards Christian
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 928eacbeb21a..99fbc7ae0f1e 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -151,7 +151,7 @@ ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,
if (ret != 0)
return ret;
if (ack == 0)
- return ret;
+ return 0;
if (!ua->dell_quirk_probed) {
ua->dell_quirk_probed = true;
@@ -171,7 +171,7 @@ ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,
}
if (!ua->dell_quirk_active)
- return ret;
+ return 0;
return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &ack, sizeof(ack));
}
