[bug report] usb: ucsi_acpi: Quirk to ack a connector change ack cmd

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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;"

    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;

	ua->dell_quirk_probed = true;

	cmd = UCSI_GET_CAPABILITY;
	ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
	...

    175 
    176         return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &ack, sizeof(ack));
    177 }

regards,
dan carpenter