Jérôme Pouiller [off-list ref] writes:

On Thursday 7 October 2021 12:49:47 CEST Kalle Valo wrote:

quoted

CAUTION: This email originated from outside of the organization. Do
not click links or open attachments unless you recognize the sender
and know the content is safe.


Kalle Valo [off-list ref] writes:

quoted

Jérôme Pouiller [off-list ref] writes:

quoted

quoted

quoted

quoted

quoted

quoted

I'm not really fond of having this kind of ASCII based parser in the
kernel. Do you have an example compressed file somewhere?

An example of uncompressed configuration file can be found here[1]. Once
compressed with [2], you get:

    {a:{a:4,b:1},b:{a:{a:4,b:0,c:0,d:0,e:A},b:{a:4,b:0,c:0,d:0,e:B},c:{a:4,b:0,c:0,d:0,e:C},d:{a:4,b:0,c:0,d:0,e:D},e:{a:4,b:0,c:0,d:0,e:E},f:{a:4,b:0,c:0,d:0,e:F},g:{a:4,b:0,c:0,d:0,e:G},h:{a:4,b:0,c:0,d:0,e:H},i:{a:4,b:0,c:0,d:0,e:I},j:{a:4,b:0,c:0,d:0,e:J},k:{a:4,b:0,c:0,d:0,e:K},l:{a:4,b:0,c:0,d:1,e:L},m:{a:4,b:0,c:0,d:1,e:M}},c:{a:{a:4},b:{a:6},c:{a:6,c:0},d:{a:6},e:{a:6},f:{a:6}},e:{b:0,c:1},h:{e:0,a:50,b:0,d:0,c:[{a:1,b:[0,0,0,0,0,0]},{a:2,b:[0,0,0,0,0,0]},{a:[3,9],b:[0,0,0,0,0,0]},{a:A,b:[0,0,0,0,0,0]},{a:B,b:[0,0,0,0,0,0]},{a:[C,D],b:[0,0,0,0,0,0]},{a:E,b:[0,0,0,0,0,0]}]},j:{a:0,b:0}}

So what's the grand idea with this braces format? I'm not getting it.

  - It allows to describe a tree structure
  - It is ascii (easy to dump, easy to copy-paste)
  - It is small (as I explain below, size matters)
  - Since it is similar to JSON, the structure is obvious to many people

Anyway, I am not the author of that and I have to deal with it.

I'm a supported for JSON like formats, flexibility and all that. But
they belong to user space, not kernel.

quoted

quoted

Usually the drivers just consider this kind of firmware configuration
data as a binary blob and dump it to the firmware, without knowing what
the data contains. Can't you do the same?

[I didn't had received this mail :( ]

The idea was also to send it as a binary blob. However, the firmware use
a limited buffer (1500 bytes) to parse it. In most of case the PDS exceeds
this size. So, we have to split the PDS before to send it.

Unfortunately, we can't split it anywhere. The PDS is a tree structure and
the firmware expects to receive a well formatted tree.

So, the easiest way to send it to the firmware is to split the tree
between each root nodes and send each subtree separately (see also the
comment above wfx_send_pds()).

Anyway, someone has to cook this configuration before to send it to the
firmware. This could be done by a script outside of the kernel. Then we
could change the input format to simplify a bit the processing in the
kernel.

I think a binary file with TLV format would be much better, but I'm sure
there also other good choises.

quoted

However, the driver has already some users and I worry that changing
the input format would lead to a mess.

You can implement a script which converts the old format to the new
format. And you can use different naming scheme in the new format so
that we don't accidentally load the old format. And even better if you
add a some kind of signature in the new format and give a proper error
from the driver if it doesn't match.

Ok. I am going to change the input format. I think the new function is
going to look like:

int wfx_send_pds(struct wfx_dev *wdev, u8 *buf, size_t buf_len)
{
     int ret;
     int start = 0;

     if (buf[start] != '{') {
             dev_err(wdev->dev, "valid PDS start with '{'. Did you forget to compress it?\n");
             return -EINVAL;
     }
     while (start < buf_len) {
             len = strnlen(buf + start, buf_len - start);
             if (len > WFX_PDS_MAX_SIZE) {
                     dev_err(wdev->dev, "PDS chunk is too big (legacy format?)\n");
                     return -EINVAL;
             }
             dev_dbg(wdev->dev, "send PDS '%s'\n", buf + start);
             ret = wfx_hif_configuration(wdev, buf + start, len);
             /* FIXME: Add error handling here */
             start += len;
     }
     return 0;

Did you read at all what I wrote above? Please ditch the ASCII format
completely.

Sorry, I read this too hastily. I just saw "buf[start] != '{'" and
assumed this is the same ASCII format, but not sure anymore. Can you
explain what changes you made now?

The script I am going to write will compute where the PDS have to be split
(this work is currently done by the driver). The script will add a
separating character (let's say '\0') between each chunk.

The driver will just have to find the separating character, send the
chunk and repeat.

I would forget ASCII altogether and implement a proper binary format
like TLV. For example, ath10k uses TLV with board-2.bin files (grep for
enum ath10k_bd_ie_type).

Also I recommend changing the file "signature" ('{') to something else
so that the driver detects incorrect formats. And maybe even use suffix
.pds2 or something like that to make it more obvious and avoid
confusion?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches