Re: [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population

["Pete Batard" <pete@akeo.ie>]

Hi Leif,

It's a disagreement. And the same goes for 3/5 & 4/5. Please see the 
note I wrote in 0/5 for the v2, because the cover letter this is usually 
the place I try to clarify elements that may throw off a maintainer, and 
that don't belong in a commit message.

Not so sound flippant here, but as long as there isn't an MTV award for 
"Most atomic codebase ever", I just don't have the time to split what I 
consider to be frivolous commits. The reasoning behind that is that I 
realistically don't consider that people are actually going to be thrown 
off by a "while I was here I also fixed an obvious typo" that got added 
into an existing commit or, most important, that even if they do, the 
amount of time that is going to be collectively wasted by people who 
might be thrown of by not having uber atomicity is not going to exceed 
the amount of time it will cost *me* to split it.

Therefore, while I do understand the desire to have an atomic commit 
history, I'm afraid that if we can't strike a balance between how much 
extra time contributors are expected to waste vs how  atomic a 
*real-life* codebase is enforced to be, if I have to split every little 
typo and stylistic fix into yet another commit, I'm simply not going to 
bother fixing typos or low hanging fruits I see any more.

Regards,

/Pete


On 2019.10.10 09:43, Leif Lindholm wrote:
> On Tue, Oct 08, 2019 at 01:38:38PM +0100, Pete Batard wrote:
>> Improve RpiFirmwareGetSerial() to derive a serial number from the
>> MAC address, in case the platform returns 0 for the serial number.
>>
>> Also fix a typo where "%s" was used instead of "%a".
> 
> I did not see a reply to my feedback on the previous round, so I'm
> unsure if this is a mistake or a disagreement?
> 
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>>   Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>> index 378c99bcba05..c2344252d2c0 100644
>> --- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>> +++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>> @@ -219,7 +219,7 @@ RpiFirmwareSetPowerState (
>>   
>>     if (!EFI_ERROR (Status) &&
>>         PowerState ^ (Cmd->TagBody.PowerState & RPI_MBOX_POWER_STATE_ENABLE)) {
>> -    DEBUG ((DEBUG_ERROR, "%a: failed to %sable power for device %d\n",
>> +    DEBUG ((DEBUG_ERROR, "%a: failed to %aable power for device %d\n",
> 
> This is clearly a bugfix, but I don't see it as being part of "Improve
> serial number population", which is what it claims to be if I view it
> wih git blame.
> 
> /
>      Leif
> 
>>         __FUNCTION__, PowerState ? "en" : "dis", DeviceId));
>>       Status = EFI_DEVICE_ERROR;
>>     }
>> @@ -393,7 +393,14 @@ RpiFirmwareGetSerial (
>>     }
>>   
>>     *Serial = Cmd->TagBody.Serial;
>> -  return EFI_SUCCESS;
>> +  // Some platforms return 0 for serial. For those, try to use the MAC address.
>> +  if (*Serial == 0) {
>> +    Status = RpiFirmwareGetMacAddress ((UINT8*) Serial);
>> +    // Convert to a more user-friendly value
>> +    *Serial = SwapBytes64 (*Serial << 16);
>> +  }
>> +
>> +  return Status;
>>   }
>>   
>>   #pragma pack()
>> -- 
>> 2.21.0.windows.1
>>