Re: [PATCH 6/7] soundwire: debugfs: add interface to read/write commands

[Pierre-Louis Bossart]

On 4/11/24 04:28, Vinod Koul wrote:
> On 05-04-24, 10:12, Pierre-Louis Bossart wrote:
>> On 4/5/24 06:45, Vinod Koul wrote:
>>> On 26-03-24, 09:01, Bard Liao wrote:
>>>> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
>>>>
>>>> We have an existing debugfs files to read standard registers
>>>> (DP0/SCP/DPn).
>>>>
>>>> This patch provides a more generic interface to ANY set of read/write
>>>> contiguous registers in a peripheral device. In follow-up patches,
>>>> this interface will be extended to use BRA transfers.
>>>>
>>>> The sequence is to use the following files added under the existing
>>>> debugsfs directory for each peripheral device:
>>>>
>>>> command (write 0, read 1)
>>>> num_bytes
>>>> start_address
>>>> firmware_file (only for writes)
>>>> read_buffer (only for reads)
>>>>
>>>> Example for a read command - this checks the 6 bytes used for
>>>> enumeration.
>>>>
>>>> cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
>>>> echo 1 > command
>>>> echo 6 > num_bytes
>>>> echo 0x50 > start_address
>>>> echo 1 > go
>>>
>>> can we have a simpler interface? i am not a big fan of this kind of
>>> structure for debugging.
>>>
>>> How about two files read_bytes and write_bytes where you read/write
>>> bytes.
>>>
>>> echo 0x50 6 > read_bytes
>>> cat read_bytes
>>>
>>> in this format I would like to see addr and values (not need to print
>>> address /value words (regmap does that too)
>>>
>>> For write
>>>
>>> echo start_addr N byte0 byte 1 ... byte N > write_bytes
>>
>> I think you missed the required extension where we will add a new
>> 'command_type' to specify which of the regular or BTP/BRA accesses is used.
>>
>> Also the bytes can come from a firmware file, it would be very odd to
>> have a command line with 32k value, wouldn't it?
> 
> ofc no one should expect that... it should be written directly from the firmware file
> 
>> I share your concern about making the interface as simple as possible,
>> but I don't see how it can be made simpler really. We have to specify
>> - read/write
>> - access type (BRA or regular)
>> - start address
>> - number of bytes
>> - a firmware file for writes
>> - a means to see the read data.
>>
>> And I personally prefer one 1:1 mapping between setting and debugfs
>> file, rather than a M:1 mapping that require users to think about the
>> syntax and which value maps to what setting. At my age I have to define
>> things that I will remember on the next Monday.
> 
> Exactly, you won't remember all the files to write to, my idea was a
> simple format or addr, N and data.. a TLV kind of structure..

So your updated proposal for a write is

echo 1 0 0x50 6 test.bin > write_bytes

Mine was

echo 1 > command
echo 0 > access
echo 0x50 > start_addr
echo 6 > num_bytes
echo test.bin > firmware
echo 1 > go

I find the last version a lot more explicit and self-explanatory. There
is no need to look-up the documentation of the list and order of arguments.
You can argue that there are just three files needed (write command,
read command, read results), but there's more work to parse arguments
and remember the arguments order.

There's also the scripting part. In my tests, I relied on scripts where
I modified one line, it's just easier to visualize than modifying one
argument in the middle of a line.

The last point is extensibility. In the existing BPT/BRA tests, the data
is sent by chunks in each frame. We know that some peripherals cannot
tolerate back-to-back transfers in contiguous frames, that would require
additional arguments to control the flow. If we have to do this with a
list of arguments, it's not straightforward to do without a versioning
scheme.

The risk of getting things wrong is not really a concern, if the
destination or number of bytes is incorrect the command will fail. It
will not cause any issues. It's a debugfs interface to inject commands,
it's expected that people will actually try to make things fail...

Again, this is NOT how the BPT/BRA protocol would be used in a real
product. The integration would rely on the codec driver initiating those
transfers. What this debugfs interface helps with is only for initial
integration tests between a host and peripheral to simulate that the
codec driver will do in the final iteration.

> What would happen if you issue go, but missed writing one of the files.
> Also I would expect you to do error checking of inputs...

Please see the patch, the inputs are checked when possible to avoid
buffer overflows, etc, but as mentioned above it's important to allow
for bad commands to be issued to test the robustness of the driver.
There is e.g. no check on the start_addr, number of bytes, the interface
allows access to any part of the peripheral memory space. That's a
feature, not a bug.