Re: [PATCH 1/2] ALSA: sscape: Cache per-card resources for board reinitialization

[Cássio Gabriel Monteiro Pires]

On 4/11/26 05:07, Takashi Iwai wrote:
> On Sat, 11 Apr 2026 06:54:29 +0200,
> Cássio Gabriel wrote:
>> +/*
>> + * Restore the SoundScape's MIDI control state after the firmware
>> + * upload has made the host interface available again.
>> + */
>> +static int sscape_restore_midi_state(struct soundscape *sscape)
>> +{
>> +	int err = 1;
>> +
>> +	guard(spinlock_irqsave)(&sscape->lock);
>> +	set_host_mode_unsafe(sscape->io_base);
>> +	err &= host_write_ctrl_unsafe(sscape->io_base, CMD_SET_MIDI_VOL, 100);
>> +	err &= host_write_ctrl_unsafe(sscape->io_base, sscape->midi_vol, 100);
>> +	err &= host_write_ctrl_unsafe(sscape->io_base, CMD_XXX_MIDI_VOL, 100);
>> +	err &= host_write_ctrl_unsafe(sscape->io_base, sscape->midi_vol, 100);
>> +	err &= host_write_ctrl_unsafe(sscape->io_base, CMD_SET_EXTMIDI, 100);
>> +	err &= host_write_ctrl_unsafe(sscape->io_base, 0, 100);
>> +	err &= host_write_ctrl_unsafe(sscape->io_base, CMD_ACK, 100);
>> +	set_midi_mode_unsafe(sscape->io_base);
>> +
>> +	return err ? 0 : -EIO;
> 
> The above means it checks only the error of the last write.
> Is it intentional?

It was meant to accumulate failures across the whole sequence, since
host_write_ctrl_unsafe() returns 0/1 and the accumulator starts at 1.

So an earlier failed write is meant to keep the final result failed, not
only the last write. But I agree the current form is kinda easy to misread.

I can rewrite to make the cumulative error handling explicit:

static int sscape_restore_midi_state(struct soundscape *sscape)
  {
        bool ok = true;

        guard(spinlock_irqsave)(&sscape->lock);
        set_host_mode_unsafe(sscape->io_base);

        if (!host_write_ctrl_unsafe(sscape->io_base, CMD_SET_MIDI_VOL, 100))
                ok = false;
        if (!host_write_ctrl_unsafe(sscape->io_base, sscape->midi_vol, 100))
                ok = false;
        if (!host_write_ctrl_unsafe(sscape->io_base, CMD_XXX_MIDI_VOL, 100))
                ok = false;
        if (!host_write_ctrl_unsafe(sscape->io_base, sscape->midi_vol, 100))
                ok = false;
        if (!host_write_ctrl_unsafe(sscape->io_base, CMD_SET_EXTMIDI, 100))
                ok = false;
        if (!host_write_ctrl_unsafe(sscape->io_base, 0, 100))
                ok = false;
        if (!host_write_ctrl_unsafe(sscape->io_base, CMD_ACK, 100))
                ok = false;

        set_midi_mode_unsafe(sscape->io_base);

        return ok ? 0 : -EIO;
  }

What you think?

-- 
Thanks,
Cássio

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature