Re: [PATCH 4/6] platform/apple: Add new Apple Mac SMC driver

[Hector Martin <marcan@xxxxxxxxx>]

On 02/09/2022 04.26, Andy Shevchenko wrote:
> On Thu, Sep 1, 2022 at 5:18 PM Russell King <rmk+kernel@xxxxxxxxxxxxxxx> wrote:
>>
>> From: Hector Martin <marcan@xxxxxxxxx>
>>
>> This driver implements support for the SMC (System Management
>> Controller) in Apple Macs. In contrast to the existing applesmc driver,
>> it uses pluggable backends that allow it to support different SMC
>> implementations, and uses the MFD subsystem to expose the core SMC
>> functionality so that specific features (gpio, hwmon, battery, etc.) can
>> be implemented by separate drivers in their respective downstream
>> subsystems.
>>
>> The initial RTKit backend adds support for Apple Silicon Macs (M1 et
>> al). We hope a backend for T2 Macs will be written in the future
>> (since those are not supported by applesmc), and eventually an x86
>> backend would allow us to fully deprecate applesmc in favor of this
>> driver.
> 
> ...
> 
>>  drivers/platform/Kconfig           |   2 +
>>  drivers/platform/Makefile          |   1 +
>>  drivers/platform/apple/Kconfig     |  49 ++++
>>  drivers/platform/apple/Makefile    |  11 +
> 
> Are you going to collect the code from, e.g., PDx86 which supports
> some apple devices here?

This driver is intended to eventually supersede hwmon/applesmc.c, once
it gets the missing features (hwmon in particular) and someone writes a
legacy x86 backend. In the meantime, it is likely to first gain support
for T2 machines, which applesmc.c does not have.

>> +       smc->msg_id = (smc->msg_id + 1) & 0xf;
> 
> % 16 will tell much cleaner of the purpose, no?

I disagree. msg_id goes in a bit field in SMC messages, and & 0xf
perfectly conveys the idea that it is limited to 4 bits.

> 
> ...
> 
>> +       while (smc->atomic_pending) {
>> +               ret = apple_rtkit_poll(smc->rtk);
>> +               if (ret < 0) {
>> +                       dev_err(smc->dev, "RTKit poll failed (%llx)", msg);
>> +                       return ret;
>> +               }
>> +               udelay(100);
>> +       }
> 
> Something from iopoll.h to be utilised?

No? Andy, I know you like to rapid-fire code reviews, but please read
and understand the code you're reviewing with context, don't just fire
off random comments based on gut feeling. This is calling through a
framework that winds up in the mailbox code and then through a callback
processes incoming messages. It's not an iopoll.

> 
>> +               return -EIO;
>> +       }
> 
> ...
> 
>> +       result = FIELD_GET(SMC_RESULT, smc->cmd_ret);
>> +       if (result != 0)
>> +               return -result;
> 
> And this is in Linux error numbering space?!

It's in some random error numbering space, and who knows how we should
map it. Maybe we should just return -EIO unconditionally, but that loses
all available information...

>> +       smc->msg_id = (smc->msg_id + 1) & 0xf;
> 
> See above. Perhaps you need a macro / inline helper for this to avoid dups.

I really don't think incrementing a 4-bit counter is an idiom that
kernel driver programmers are going to find confusing, and gratuituous
macro use just makes the code harder to read.

>> +       do {
>> +               if (wait_for_completion_timeout(&smc->cmd_done,
>> +                                               msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) {
>> +                       dev_err(smc->dev, "Command timed out (%llx)", msg);
>> +                       return -ETIMEDOUT;
>> +               }
>> +               if (FIELD_GET(SMC_ID, smc->cmd_ret) == smc->msg_id)
>> +                       break;
> 
>> +               dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n",
>> +                       smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret));
> 
> Guaranteed to flood the logs...

No.

> 
>> +       } while(1);
> 
> ...with such a conditional.

It's waiting on a completion every loop. And the message ID is 4 bits,
so the most you will ever get is 15 messages (but in practice what
happens is that if this goes wrong, it just times out on the next loop,
and generally everything breaks anyway, and all of it is indicative of a
major bug.

Again, please read the code, don't just go "dev_err() in a while(1), bad!".

>> +       if (size <= 4)
>> +               memcpy(buf, &rdata, size);
>> +       else
>> +               memcpy_fromio(buf, smc->shmem.iomem, size);
> 
> This is unclear why plain memcpy() for the small size and what are the
> side effects of the memory.

It's on the stack. Can you *please* read more than two lines of context
while reviewing? This is getting tiring. You do this *all* the time.
It's even taking the address of the variable right there, you didn't
even need to read past that one line to understand it.

> Maybe you wanted memremap() instead of
> ioremap() to begin with?

ioremap() uses a completely different mapping mode to memremap(). They
are not interchangeable and memremap will not work here.

>> +       if (res.end < res.start || !resource_contains(smc->sram, &res)) {
> 
> Is it a reimplementation of something like resource_intersect() and Co?

I don't know, is it? Can you please make specific suggestions instead of
randomly throwing around ideas without checking whether they actually
make sense?

>> +       bfr->iomem = smc->sram_base + (res.start - smc->sram->start);
> 
> Isn't it better to write as
> 
>   res.start + (base - start)
> 
> ?

smc->sram_base is the iomem base of the sram, smc->sram->start is the
physical address of the sram, and res.start is the physical address of
the area the SMC is referencing.

__iomem + (physaddr - physaddr) == __iomem + offset == __iomem

You are proposing:

physaddr + (__iomem - physaddr)

which not only does not make sense, it's undefined behavior in C, since
you are doing pointer arithmetic exceeding the object bounds of the
iomem map.

>> +       smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
> 
>> +       if (!smc->sram)
>> +               return dev_err_probe(dev, EIO,
>> +                                    "No SRAM region");
> 
> Dup, the below does this message for you.

There is a difference between "the DT is missing the SRAM resource" and
"couldn't map the SRAM resource for some reason". Why merge them into
one error?

> 
>> +       smc->sram_base = devm_ioremap_resource(dev, smc->sram);
>> +       if (IS_ERR(smc->sram_base))
>> +               return dev_err_probe(dev, PTR_ERR(smc->sram_base),
>> +                                    "Failed to map SRAM region");
> 
> Don't we have devm_platform_ioremap_resource_byname() ?

Except I use smc->sram elsewhere.

You cannot review patches by ignoring everything but the two lines of
context you are staring at right now. Before complaining about
something, please *look* to see if there is a good reason for the code
to be the way it is.

> ...
> 
>> +typedef u32 smc_key;
> 
> Why?!

Because SMC keys are 32 bits and giving them their own type makes it
explicit what is supposed to be an SMC key.

> 
>> +#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
> 
> If s is a byte buffer, the above is NIH get_unaligned_be32(). Or in
> case of alignment be32_to_cpu() with respective type (__be32) to be
> used.

Except this is a constexpr, and get_unaligned_be32 is not.

s is a 32-bit identifier that happens to consist of 4 8-bit character
ASCII fields. This is a macro to generate them from ASCII strings, as
compile-time constants. You'd have figured that out if you read ahead to
where it is used, instead of continuing your two-line-context review.

>> +#define apple_smc_write_flag apple_smc_write_u8
> 
> Why is it needed?

Because some SMC keys are boolean flags, and the ABI is identical to u8
but the logical semantics are not, and the code is clearer when it makes
it explicit that the value is supposed to be a boolean, not just an
arbitrary 8-bit integer.

Andy, no offense, but you drive-by everything I try to upstream (or
author in this case) and half of your suggestions are wrong and I have
to waste my time explaining why, and most of the rest are negligible
style nitpicks. Every now and then you point out some useful kernel
function that I didn't know about, but your signal to noise rate is
terrible. Please put some effort into your reviews. It feels like you're
on some kind of quest to review as much code as possible, without the
slightest care for quality.

- Hector