Message-ID: <CAMF+KebyMbv_t1-BvjfcXRBK-hGo=o0-Nt3LgKkUyBLfmV_4bg@mail.gmail.com>
Date: Thu, 9 Jan 2025 22:33:30 +0100
From: Joshua Grisham <josh@...huagrisham.com>
To: Thomas Weißschuh <thomas@...ch.de>
Cc: Joshua Grisham <josh@...huagrisham.com>, ilpo.jarvinen@...ux.intel.com, 
	hdegoede@...hat.com, W_Armin@....de, platform-driver-x86@...r.kernel.org, 
	corbet@....net, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] platform/x86: samsung-galaxybook: Add
 samsung-galaxybook driver

Hi Thomas,

Den ons 8 jan. 2025 kl 23:07 skrev Thomas Weißschuh <thomas@...ch.de>:
>
> If it works, then leave it as is.
> To exact warning would have been nice though :-)
>
> Did you try :ref:`userspace-api/sysfs-platform_profile`?
>

Just tried this specifically again and the warning was:
./Documentation/admin-guide/laptops/samsung-galaxybook.rst:72:
WARNING: undefined label: 'userspace-api/sysfs-platform_profile'
[ref.ref]

As it seems to work exactly as intended with only having the path as
clear text (a link is added in both pdf and html plus the title of the
target page is displayed as the link text) then I will leave as-is for
now but please say if you would like for me to try anything else!

> > > > +static int galaxybook_acpi_method(struct samsung_galaxybook *galaxybook, acpi_string method,
> > > > +                               struct sawb *in_buf, size_t len, struct sawb *out_buf)
> > >
> > > in_buf and out_buf are always the same.
> > >
> > > > +{
> > > > +     struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> > > > +     union acpi_object in_obj, *out_obj;
> > > > +     struct acpi_object_list input;
> > > > +     acpi_status status;
> > > > +     int err;
> > > > +
> > > > +     in_obj.type = ACPI_TYPE_BUFFER;
> > > > +     in_obj.buffer.length = len;
> > > > +     in_obj.buffer.pointer = (u8 *)in_buf;
> > > > +
> > > > +     input.count = 1;
> > > > +     input.pointer = &in_obj;
> > > > +
> > > > +     status = acpi_evaluate_object_typed(galaxybook->acpi->handle, method, &input, &output,
> > > > +                                         ACPI_TYPE_BUFFER);
> > > > +
> > > > +     if (ACPI_FAILURE(status)) {
> > > > +             dev_err(&galaxybook->acpi->dev, "failed to execute method %s; got %s\n",
> > > > +                     method, acpi_format_exception(status));
> > > > +             return -EIO;
> > > > +     }
> > > > +
> > > > +     out_obj = output.pointer;
> > > > +
> > > > +     if (out_obj->buffer.length != len || out_obj->buffer.length < SAWB_GUNM_POS + 1) {
> > > > +             dev_err(&galaxybook->acpi->dev, "failed to execute method %s; "
> > > > +                     "response length mismatch\n", method);
> > > > +             err = -EPROTO;
> > > > +             goto out_free;
> > > > +     }
> > > > +     if (out_obj->buffer.pointer[SAWB_RFLG_POS] != RFLG_SUCCESS) {
> > > > +             dev_err(&galaxybook->acpi->dev, "failed to execute method %s; "
> > > > +                     "device did not respond with success code 0x%x\n",
> > > > +                     method, RFLG_SUCCESS);
> > > > +             err = -ENXIO;
> > > > +             goto out_free;
> > > > +     }
> > > > +     if (out_obj->buffer.pointer[SAWB_GUNM_POS] == GUNM_FAIL) {
> > > > +             dev_err(&galaxybook->acpi->dev,
> > > > +                     "failed to execute method %s; device responded with failure code 0x%x\n",
> > > > +                     method, GUNM_FAIL);
> > > > +             err = -ENXIO;
> > > > +             goto out_free;
> > > > +     }
> > > > +
> > > > +     memcpy(out_buf, out_obj->buffer.pointer, len);
> > >
> > > Nit: This memcpy() could be avoided by having the ACPI core write directly
> > > into out_buf. It would also remove the allocation.
> > >
> >
> > Now I have replaced in_buf and out_buf with just one parameter, buf.
> > Now it feels like I cannot write directly to it (since I am reusing
> > the same buf as the outgoing value) so have left the memcpy in place.
> > I guess I would need to choose to have 2 buffers or use one and do a
> > memcpy at the end like this (which is how I have it now in my v5
> > draft) .. am I thinking wrong here and/or is there a preference
> > between the two alternatives? I can just for now say that "usage" of
> > this function in all of the other functions feels easier to just have
> > one buffer... :)
>
> I'm not sure if there is a preference.
>
> But why can't you modify the buffer if it is shared between input and
> output? The caller already has to accept that its buffer will be
> overwritten.
> If it is overwritten once or twice should not matter.
>
> But maybe I'm misunderstanding.
>

There is a very non-zero chance that I am trying to do this completely
wrong ;) but basically if I swap

struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
to
struct acpi_buffer output = {len, buf};
or even
struct acpi_buffer output = {len, (u8 *)buf};

Then I am getting return code of AE_BUFFER_OVERFLOW when trying to
call the method, even though when using ACPI_ALLOCATE_BUFFER len is
always the same as the allocated out_obj->buffer.length.

I have also tried a few variations of using a union acpi_object and
setting the buffer member properties etc but always I am getting
AE_BUFFER_OVERFLOW so it seems like something is a bit off on the
length or I am using the wrong types or something. I have tried
looking through the entire tree and using ACPI_ALLOCATE_BUFFER is
almost universal so it is tough to find examples to try and understand
what else might be possible without really digging deep into the ACPI
tree.

If you know right off the top of your head then please feel free to
mention, otherwise I will keep the new buffer and do a memcpy and free
the newly allocated buffer at the end for the time being!

Thanks again!
Joshua

Powered by blists - more mailing lists