linux-kernel - Re: [PATCH v2] ASoC: tas2783: Add source files for tas2783 soundwire driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 17 Aug 2023 11:28:25 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Baojun Xu <baojun.xu@...com>, lgirdwood@...il.com, perex@...ex.cz,
        tiwai@...e.com, shenghao-ding@...com, kevin-lu@...com,
        krzysztof.kozlowski@...aro.org, rf@...nsource.cirrus.com,
        shumingf@...ltek.com, herve.codina@...tlin.com,
        povik+lin@...ebit.org, ryans.lee@...log.com,
        ckeepax@...nsource.cirrus.com, sebastian.reichel@...labora.com,
        fido_max@...ox.ru, wangweidong.a@...nic.com,
        linux-kernel@...r.kernel.org, alsa-devel@...a-project.org,
        peeyush@...com, navada@...com, tiwai@...e.de,
        mengdong.lin@...el.com
Subject: Re: [PATCH v2] ASoC: tas2783: Add source files for tas2783 soundwire
 driver

On 8/17/23 10:12, Mark Brown wrote:
> On Thu, Aug 17, 2023 at 09:17:50AM -0500, Pierre-Louis Bossart wrote:
> 
>>> +		goto out;
>>> +	}
>>> +	/* Read the primary device as the whole */
>>
>> I can't figure out what this comment means
> 
> It's part of...
> 
>>> +		dev_err(tas_dev->dev, "%s, regmap doesn't exist.\n",
>>> +			__func__);
>>> +		return -EINVAL;
>>> +	}
>>> +	/* Read the primary device */
>>
>> What is a primary device?
> 
> ...a thing where they're trying to present multiple devices as a unified
> device with grouped control, it looks like there's some hardware support
> for this.

Let me clarify the comment: SDCA peripheral can have multiple functions,
each with its own address space and can operate independently. So I am
just trying to have clarity on what 'device' means here.

>>> +	/* Failed got calibration data from EFI. */
> 
>> I don't get what the dependency on EFI is. First time I see a codec
>> needing this.
> 
>> Please describe in details what you are trying to accomplish.
> 
> It seems fairly normal to store calibration details in the device
> firmware?

No objection on the device firmware, but why use an EFI variable?

There is on-going work to standardize with ACPI, and there's also a
request_firmware(). Not sure what the direction is to read from an EFI
variable. I've been in SDCA circles since the beginning and never heard
about this, ever. I am not saying it's bad, just surprised and curious
on a 3rd way of getting information needed for initialization.

>>> +	if (crc == tmp_val[21]) {
>>> +		time64_to_tm(tmp_val[20], 0, tm);
>>> +		dev_dbg(tas_priv->dev, "%4ld-%2d-%2d, %2d:%2d:%2d\n",
>>> +			tm->tm_year, tm->tm_mon, tm->tm_mday,
>>> +			tm->tm_hour, tm->tm_min, tm->tm_sec);
> 
>> What is this about? Why would a codec care about time?
> 
> I can see someone finding it helpful to confirm when the calibration data
> that got applied was generated to make sure they're testing/using what
> they thought they were.

Ah yes, I missed that. I wasn't sure if this was a log on when the
calibration finished, if this is a log on when the calibration data was
generated that's a different story indeed.

>> In addition, it's rather surprising that on attachment there is not a
>> single regmap access?
> 
> Don't know if there's something different with Soundwire but for I2C/SPI
> devices it's a reasonable pattern to only actually start initialising
> the registers when the device actually becomes active.  Not checked that
> this is actually happening.

that's precisely the point, there's an io_init() routine which is when
the peripheral is attached on the bus and the earliest time when the
registers can be initialized.

But there isn't a single initialization happening, which is different to
all existing SoundWire codec drivers. Maybe it's fine, I am just asking
the question if this was intentional.