On Thu, May 07, 2020 at 09:23:21PM +0300, Andy Shevchenko wrote:
> On Wed, May 6, 2020 at 3:16 PM <malattia@xxxxxxxx> wrote:
> >
> > From: Mattia Dongili <malattia@xxxxxxxx>
> >
> > After commit 6d232b29cfce ("ACPICA: Dispatcher: always generate buffer
> > objects for ASL create_field() operator") ACPICA creates buffers even
> > when new fields are small enough to fit into an integer.
> > Many SNC calls counted on the old behaviour.
> > Since sony-laptop already handles the INTEGER/BUFFER case in
> > sony_nc_buffer_call, switch sony_nc_int_call to use its more generic
> > function instead.
>
> Thank you for an update.
>
> ...
>
> The patches require prefix, "platform/x86: sony-laptop: ".
> I fixed it for now.
>
> ...
>
> > + // do nothing
>
> Use C99 comment style, please.
Maybe you mean *don't* use C99 comments?
> ...
>
> > +static int sony_nc_int_call(acpi_handle handle, char *name, int *value, int
> > + *result)
> > +{
>
> > + if (result)
> > + *result = 0;
>
> I didn't get this part. Does it mean we always have to reset result?
> Sounds like a design issue (usual pattern is to ignore output in case
> of error by caller and to avoid touching output by callee)
The sony_nc_buffer_call copies data into *result using memcopy, if there
is less data than sizeof(*result), it may leave some bytes uninitialzed.
I agree that resetting results here smells.
The reset is still necessary, but I'm turning to use memset inside
sony_nc_buffer_call if there's a result to copy.
That should respect the pattern you described.
> > + return sony_nc_buffer_call(handle, name, (u64 *)value, result,
> > + sizeof(*result));
>
> Oh, this way for troubles. You supply pointer to int and force it to
> be u64. Not good. See how above function has been implemented in this
> sense.
That's right, thanks for pointing it out.
v4 incoming.
--
mattia
:wq!
