Re: [PATCH v4 04/17] ASoC: Intel: avs: Inter process communication

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-03-09 11:10 PM, Pierre-Louis Bossart wrote:
> 
>> +/*
>> + * struct avs_dsp_ops - Platform-specific DSP operations
>> + *
>> + * @power: Power on or off DSP cores
>> + * @reset: Enter or exit reset state on DSP cores
>> + * @stall: Stall or run DSP cores
> 
> nit-pick: the description sounds like boolean states. add "callback to"


The title i.e. struct's description reads: *DSP operations*. And thus I 
believe the *DSP operation* prefix applies to everything below 
implicitly. Otherwise we have spam of: "A callback to do <this and that>".

>> + * @irq_handler: Top half of IPC servicing
>> + * @irq_thread: Bottom half of IPC servicing
>> + * @int_control: Enable or disable IPC interrupts
> 
> callback to ...


Ditto.

>> + */
>>   struct avs_dsp_ops {
>>       int (* const power)(struct avs_dev *, u32, bool);
>>       int (* const reset)(struct avs_dev *, u32, bool);
>>       int (* const stall)(struct avs_dev *, u32, bool);
>> +    irqreturn_t (* const irq_handler)(int, void *);
>> +    irqreturn_t (* const irq_thread)(int, void *);
>> +    void (* const int_control)(struct avs_dev *, bool);
>>   };
> 
>> +/*
>> + * struct avs_ipc - DSP IPC context
>> + *
>> + * @dev: PCI device
>> + * @rx: Reply message cache
> 
> cache? I find this confusing, what are you trying to say here?


The goal of that member is to reduce amount of memory allocations needed 
during message processing. That's why it's called cache.

>> + * @default_timeout_ms: default message timeout in MS
>> + * @ready: whether firmware is ready and communication is open
>> + * @rx_completed: whether RX for previously sent TX has been received
>> + * @rx_lock: for serializating manipulation of rx_* fields
> 
> typo: serializing


Ack.

>> + * @msg_lock: for synchronizing request handling
>> + * @done_completion: DONE-part of IPC i.e. ROM and ACKs from FW
>> + * @busy_completion: BUSY-part of IPC i.e. receiving responses from FW
>> + */
>> +struct avs_ipc {
>> +    struct device *dev;
>> +
>> +    struct avs_ipc_msg rx;
>> +    u32 default_timeout_ms;
>> +    bool ready;
>> +
>> +    bool rx_completed;
>> +    spinlock_t rx_lock;
>> +    struct mutex msg_mutex;
>> +    struct completion done_completion;
>> +    struct completion busy_completion;
>> +};
>> +
>> +#define AVS_EIPC    EREMOTEIO
> 
> I don't recall if I already mentioned this but I don't see the need for 
> an intermediate redefinition of a Linux error code.


Hmm.. I've already explained that code makes use of this macro to 
differentiate between IPC protocol errors and *other* errors. Otherwise, 
if code gets changed, every single usage -EREMOTEIO will need to be 
addressed, not just AVS_EIPC declaration.

>> +/*
>> + * IPC handlers may return positive value (firmware error code) what 
>> denotes
>> + * successful HOST <-> DSP communication yet failure to process 
>> specific request.
>> + *
>> + * Below macro converts returned value to linux kernel error code.
>> + * All IPC callers MUST use it as soon as firmware error code is 
>> consumed.
>> + */
>> +#define AVS_IPC_RET(ret) \
>> +    (((ret) <= 0) ? (ret) : -AVS_EIPC)
>> +
>> +static inline void avs_ipc_err(struct avs_dev *adev, struct 
>> avs_ipc_msg *tx,
>> +                   const char *name, int error)
>> +{
>> +    /*
>> +     * If IPC channel is blocked e.g.: due to ongoing recovery,
>> +     * -EPERM error code is expected and thus it's not an actual error.
>> +     */
>> +    if (error == -EPERM)
>> +        dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name,
> 
> %# adds the 0x for you.
> 
>> +            tx->glb.primary, tx->glb.ext.val, error);
>> +    else
>> +        dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name,
>> +            tx->glb.primary, tx->glb.ext.val, error);
>> +}
> 
>> +static void avs_dsp_process_notification(struct avs_dev *adev, u64 
>> header)
>> +{
>> +    struct avs_notify_mod_data mod_data;
>> +    union avs_notify_msg msg = AVS_MSG(header);
>> +    size_t data_size = 0;
>> +    void *data = NULL;
>> +
>> +    /* Ignore spurious notifications until handshake is established. */
> 
> there's no handshake here, just an initial notification after which the 
> IPC protocol can start?


There is no *real* handshake yes, but the FW_READY is sent as a response 
to us (the driver) programming ADSP cores and loading firmware.


Regards,
Czarek