On Mon, Oct 30, 2023 at 04:05:09PM -0500, Pierre-Louis Bossart wrote:
On 10/30/23 12:40, Pierre-Louis Bossart wrote:
+static bool tas2783_readable_register(struct device *dev, unsigned int reg) +{
- switch (reg) {
- case 0x000 ... 0x080: /* Data port 0. */
No, this is wrong. All the data port 'standard' registers are "owned" by the SoundWire core and handled during the port prepare/configure/bank switch routines. Do not use them for regmap.
This seems to come up a moderate amount and is an understandable thing to do - could you (or someone else who knows SoundWire) perhaps send a patch for the regmap SoundWire integration which does some validation here during registration and at least prints a warning?
Good suggestion, we could indeed check that the registers are NOT in the range [0,0xBF] for all ports - only the range [0xC0..FF] is allowed for implementation-defined values. I'll try to cook something up.
After checking, the following ranges are invalid for codec drivers:
for address < 0x1000 LSB = 0x00 - 0xBF standard or reserved
0x1800 – 0x1FFF reserved 0x48000000 - 0xFFFFFFFF reserved
That's a huge range... I think the concern is more the standard ranges than the reserved ranges isn't it? That's the bit that the SoundWire core or other generic code will be actively managing.
is the recommendation to check the regmap_config and its 'yes_ranges'?
Presumably if the range_min or range_max is within the invalid values above then the configuration can be tagged as problematic in the dmesg log or rejected with an error code?
That would work for drivers that use that, but note that drivers can just provide a function here so you pretty much need to probe each individual register. It's done that way because we figured when the interface was originally defined that the compiler would probably generate better code from a switch statement in most cases than us trying to fine tune an algorithm. Probing like that is going to be unsustaniable for the full ranges above but shouldn't be too bad for the potentially standard bits, we could guard it with a config option if it's noticable.
Another option would be a kselftest that looks at the readable registers in debugfs, we do build up a cache of the readable ranges there when the access map or register contents are first read. That's potentially less visible but OTOH easier to ask for on review, I was debating asking for mixer-test output on all CODEC driver submissions (similar to what the media subsystem does with their validation suite).