Message-ID: <ZDsPGYJjCNmtizKk@nixie71>
Date:   Sat, 15 Apr 2023 15:54:49 -0500
From:   Jeff LaBundy <jeff@...undy.com>
To:     Fred Treven <Fred.Treven@...rus.com>
Cc:     Charles Keepax <ckeepax@...nsource.cirrus.com>,
        "dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>,
        Ben Bright <Ben.Bright@...rus.com>,
        James Ogletree <James.Ogletree@...rus.com>,
        "lee@...nel.org" <lee@...nel.org>,
        "jdelvare@...e.de" <jdelvare@...e.de>,
        "joel@....id.au" <joel@....id.au>,
        "cy_huang@...htek.com" <cy_huang@...htek.com>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "eajames@...ux.ibm.com" <eajames@...ux.ibm.com>,
        "ping.bai@....com" <ping.bai@....com>,
        "msp@...libre.com" <msp@...libre.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "bartosz.golaszewski@...aro.org" <bartosz.golaszewski@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "patches@...nsource.cirrus.com" <patches@...nsource.cirrus.com>
Subject: Re: [PATCH 1/2] Input: cs40l26: Support for CS40L26 Boosted Haptic
 Amplifier

Hi Fred,

On Fri, Apr 14, 2023 at 08:51:48PM +0000, Fred Treven wrote:
> 
> >> +
> >> + return cs40l26_probe(cs40l26, pdata);
> >> +}
> >> +
> >> +static void cs40l26_i2c_remove(struct i2c_client *client)
> >> +{
> >> + struct cs40l26_private *cs40l26 = i2c_get_clientdata(client);
> >> +
> >> + cs40l26_remove(cs40l26);
> >> +}
> >> +
> >> +static struct i2c_driver cs40l26_i2c_driver = {
> >> + .driver = {
> >> + .name = "cs40l26",
> >> + .of_match_table = cs40l26_of_match,
> >> + .pm = &cs40l26_pm_ops,
> > 
> > Please guard this with the new pm_sleep_ptr(), as not all platforms would
> > define CONFIG_PM. More comments in the core driver.
> 
> Understood, and I certainly agree with this change. One thing I’m unsure of is what the effect would be on the driver if CONFIG_PM is not set in the .config. Surely, this driver would not work as expected right? Should I add a dependency in the kconfig to avoid building the driver without CONFIG_PM?

The PM core stubs out dummy functions for most, if not all of the API used here
for the #if !CONFIG_PM case. Please test to be sure, but it's likely the driver
still compiles without CONFIG_PM set.

However, you should evaluate the driver's functionality during this case. At a
minimum, the device should still be useable but simply not hibernate.

> > 
> >> +{
> >> + size_t len_words = len_bytes / sizeof(__be32);
> >> + struct cs_dsp_coeff_ctl *ctl;
> >> + __be32 *val;
> >> + int i, ret;
> >> +
> >> + ctl = cs_dsp_get_ctl(dsp, name, WMFW_ADSP2_XM, algo_id);
> >> + if (IS_ERR_OR_NULL(ctl)) {
> >> + dev_err(dsp->dev, "Failed to find fw ctl %s\n", name);
> >> + return -ENOENT;
> >> + }
> >> +
> >> + val = kzalloc(len_bytes, GFP_KERNEL);
> >> + if (!val)
> >> + return -ENOMEM;
> >> +
> >> + for (i = 0; i < len_words; i++)
> >> + val[i] = cpu_to_be32(buf[i]);
> >> +
> >> + ret = cs_dsp_coeff_write_ctrl(ctl, off_words, val, len_bytes);
> >> + if (ret)
> >> + dev_err(dsp->dev, "Failed to write fw ctl %s: %d\n", name, ret);
> >> +
> >> + kfree(val);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static inline int cs40l26_fw_ctl_write(struct cs_dsp *dsp, const char * const name,
> >> + unsigned int algo_id, u32 val)
> >> +{
> >> + return cs40l26_fw_ctl_write_raw(dsp, name, algo_id, 0, sizeof(u32), &val);
> >> +}
> >> +
> >> +static int cs40l26_fw_ctl_read_raw(struct cs_dsp *dsp, const char * const name,
> >> + unsigned int algo_id, unsigned int off_words, size_t len_bytes, u32 *buf)
> >> +{
> >> + size_t len_words = len_bytes / sizeof(u32);
> >> + struct cs_dsp_coeff_ctl *ctl;
> >> + int i, ret;
> >> +
> >> + ctl = cs_dsp_get_ctl(dsp, name, WMFW_ADSP2_XM, algo_id);
> >> + if (IS_ERR_OR_NULL(ctl)) {
> >> + dev_err(dsp->dev, "Failed to find fw ctl %s\n", name);
> >> + return -ENOENT;
> >> + }
> >> +
> >> + ret = cs_dsp_coeff_read_ctrl(ctl, off_words, buf, len_bytes);
> >> + if (ret) {
> >> + dev_err(dsp->dev, "Failed to read fw ctl %s: %d\n", name, ret);
> >> + return ret;
> >> + }
> >> +
> >> + for (i = 0; i < len_words; i++)
> >> + buf[i] = be32_to_cpu(buf[i]);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static inline int cs40l26_fw_ctl_read(struct cs_dsp *dsp, const char * const name,
> >> + unsigned int algo_id, u32 *buf)
> >> +{
> >> + return cs40l26_fw_ctl_read_raw(dsp, name, algo_id, 0, sizeof(u32), buf);
> >> +}
> > 
> > None of these four functions seem particularly specific to L26; is there any
> > reason they don't belong in cs_dsp or wm_adsp? In fact, some of the functions
> > throughout those drivers seem to be doing similar work.
> > 
> > Maybe out of scope for this particular submission, but is there not any room
> > for re-use here?
> > 
> I wanted to avoid making too many changes to the firmware drivers in this initial submission, and I think I’d like to keep it as-is for now, but I think moving this combination to cs_dsp is the right move. Let me know if you feel that it would be better to make the change now rather than later.

I think it's fine to clear this submission first, then look for ways to
optimize later.

> 
> > On Tue, Apr 11, 2023 at 09:27:08AM +0000, Charles Keepax wrote:
> >> On Mon, Apr 10, 2023 at 07:31:56PM -0500, Jeff LaBundy wrote:
> >>> On Mon, Apr 10, 2023 at 08:56:34AM +0000, Charles Keepax wrote:
> >>>> On Sat, Apr 08, 2023 at 10:44:39PM -0500, Jeff LaBundy wrote:
> >>>> I would far rather not have every single attempt to communicate
> >>>> with the device wrapped in a retry if the communication failed
> >>>> incase the device is hibernating. It seems much cleaner, and less
> >>>> likely to risk odd behaviour, to know we have brought the device
> >>>> out of hibernation.
> >> 
> >>> A common way to deal with this is that of [1], where the bus calls
> >>> are simply wrapped with all retry logic limited to two places (read
> >>> and write). These functions could also print the register address
> >>> in case of failure, solving the problem of having dozens of custom
> >>> error messages thorughout the driver.
> >> 
> >> I suspect this really comes down to a matter of taste, but my
> >> thoughts would be that the code is shorter that way, but not
> >> necessarily simpler. This feels far more error prone and likely
> >> to encounter issues where the device hibernates at a time someone
> >> hadn't properly thought through. I am far more comfortable with
> >> the device is blocked from hibernating whilst the driver is
> >> actively engaged with it and it keeps any special handling for
> >> exiting hibernate in one place.
> > 
> > Fair enough. I do concede that having this control in the driver as
> > opposed to DSP FW is more nimble and makes it easier to respond to
> > customer issues; I'm sure your battle scars will agree :)
> 
> I concur with Charles here, and it seems like you’re also ok with this so I will leave it as-is.

I'm OK with it; it's simply not my first choice. I'm in the "less code is
the best code" camp, so I'm biased toward giving the silicon the benefit
of the doubt and letting it autosuspend on its own. This is how bare-metal
platforms are likely to use the device, and probably how it was envisioned
to be used. And if there truly is an issue with hibernation mode, it seems
you would eventually stumble upon it whether the kernel or the DSP FW is
managing the device's state. That's just my $.02.

That being said, there is nothing functionally incorrect about the current
implementation and since you are the one who is ultimately responsible for
supporting the driver, I think you should stick with what makes you most
comfortable and agile. As I mention, it can be to your advantage to have
complete explicit control over the device from the kernel.

Please do add some comments to describe how the device's state is managed
however, particularly with respect to GPIO triggers. I was also confused
as to why the kernel autosuspend timeout (2000 ms) is greater than the
device's standby-to-hibernation timeout (100 ms) because I naively assumed
the opposite would be true.

> 
> 
> >>> +static int cs40l26_irq_update_mask(struct cs40l26_private *cs40l26, u32 reg, u32 val, u32 bit_mask)
> >>> +{
> >>> + u32 eint_reg, cur_mask, new_mask;
> >>> + int ret;
> >>> +
> >>> + if (reg == CS40L26_IRQ1_MASK_1) {
> >>> + eint_reg = CS40L26_IRQ1_EINT_1;
> >>> + } else if (reg == CS40L26_IRQ1_MASK_2) {
> >>> + eint_reg = CS40L26_IRQ1_EINT_2;
> >>> + } else {
> >>> + dev_err(cs40l26->dev, "Invalid IRQ mask reg: 0x%08X\n", reg);
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + ret = regmap_read(cs40l26->regmap, reg, &cur_mask);
> >>> + if (ret) {
> >>> + dev_err(cs40l26->dev, "Failed to get IRQ mask\n");
> >> 
> >> Having a custom error message for every possible failed register read
> >> does not ultimately aid in debugging and unnecessarily grows the size
> >> of the driver.
> >> 
> >> If a specific message is absolutely necessary, then add wrappers around
> >> regmap_read/write and print the failed address. However, this does not
> >> seem necessary either. Simply propagate the error code all the way up
> >> to the caller, whether it is probe or a sysfs attribute.
> >> 
> >> Stated another way:
> >> 
> >> error = regmap_...(...);
> >> if (error)
> >> return error;
> >> 
> > 
> > Not sure I feel super strongly on this one, but I do find when
> > debugging issues on drivers that do this that I usually end up
> > adding the printks back in.
> 
> I went ahead and implemented the change Jeff suggested here; it does streamline the driver to a good degree, and I think it’s worth making the adjustment.
> 
> >>> +static int cs40l26_dsp_state_get(struct cs40l26_private *cs40l26, u8 *state)
> >>> +{
> >>> + bool mutex_available = !mutex_is_locked(&cs40l26->dsp.pwr_lock);
> >> 
> >> This is dangerous and a sign that locks are not properly managed. What would
> >> be a case where you do not know the state of the lock upon entering this function?
> >> If you do not know whether the mutex is locked inside this function, it is not the
> >> proper place to grab it.
> >> 
> > 
> > Yes I whole heartedly agree here this should not be done this
> > way.
> 
> I certainly understand the concern here, but I wanted to provide some context. Since cs40l26_dsp_state_get() is called both in the cs_dsp_pre_run() callback as well as outside of this, there are instances where pwr_lock needs to be grabbed and times when it is already taken (in the instance of the callback invocation). Because of this, attempting to take the lock during pre_run causes a deadlock. I felt that it was quite clear when the lock would be available and when it wouldn’t be and to avoid the deadlock, I checked whether or not the lock was available. What do y’all feel is the best way to handle this? Some separate variable to determine if we are in the pre_run sequence or not? A separate function for dsp_state_get() for when it is not invoked from the callback?

This is essentially my point. As a rule of thumb, if it takes this much
explanation to convey the design intent, it's probably unsafe and ripe
for bugs as others innocently contribute to this driver.

A second rule of thumb is that for proper encapsulation, the state of
the outside world should be the same regardless of how this function is
entered. Your last idea is the proper solution, for example:

static int __cs40l26_dsp_state_get(...)
{
	/* Mutex is assumed to be locked here. */

	[...]

	return error;
}

static int cs40l26_dsp_state_get(...)
{
	mutex_lock(...);

	error = __cs40l26_dsp_state_get(...);

	mutex_unlock(...);

	return error;
}

If you need to get the DSP state in some other function where the lock is
held, just call __cs40l26_dsp_state_get(). Another advantage to using a
"helper" function is that the mutex is easily unlocked in both passing or
failing cases without complex conditional blocks or goto statements.

> 
> > 
> >> +
> >> + cs40l26_pm_runtime_teardown(cs40l26);
> >> +
> >> + if (cs40l26->dsp.running)
> >> + cs_dsp_stop(&cs40l26->dsp);
> >> + if (cs40l26->dsp.booted)
> >> + cs_dsp_power_down(&cs40l26->dsp);
> >> + if (&cs40l26->dsp)
> >> + cs_dsp_remove(&cs40l26->dsp);
> >> +
> >> + if (cs40l26->vibe_workqueue) {
> >> + cancel_work_sync(&cs40l26->erase_work);
> >> + cancel_work_sync(&cs40l26->set_gain_work);
> >> + cancel_work_sync(&cs40l26->upload_work);
> >> + cancel_work_sync(&cs40l26->vibe_start_work);
> >> + cancel_work_sync(&cs40l26->vibe_stop_work);
> >> + destroy_workqueue(cs40l26->vibe_workqueue);
> >> + }
> >> +
> >> + mutex_destroy(&cs40l26->lock);
> > 
> > This ultimately does nothing.
> 
> Could you please clarify a bit what you mean here? Does the mutex not need to be destroyed explicitly? What about the work queue? Is it canceled/destroyed automatically when the module is removed? Are the cs_dsp functions not necessary to gracefully stop the DSP etc.?

Sorry, I meant that mutex_destroy() is unnecessary; it's an empty function
unless mutex debugging is enabled. There is no need to take any action on
the mutex as the driver is torn down.

You do need to cancel any pending work to prevent use-after-free conditions;
the core does not do this for you. You don't necessarily need to destroy the
work queue, however, because it seems all of the means to queue work will be
gone by the time the device-managed cs40l26 pointer is freed.

> 
> In v2 of this patch, I plan to implement all of the suggestions in your comments that I didn’t question or disagree with here. Thanks for your in-depth analysis.
> 
> Best regards,
> Fred
> 
> 

Kind regards,
Jeff LaBundy

Powered by blists - more mailing lists