Re: [PATCH v6 2/4] firmware: ti_sci: add support for restoring IRQs during resume

[Nishanth Menon <nm@ti.com>]

On 15:16-20260505, Thomas Richard wrote:

[...]

> > 
> >> +
> >> +	desc = &irq->desc;
> >> +	desc->valid_params = valid_params;
> >> +	desc->src_id = src_id;
> >> +	desc->src_index = src_index;
> >> +	desc->dst_id = dst_id;
> >> +	desc->dst_host_irq = dst_host_irq;
> >> +	desc->ia_id = ia_id;
> >> +	desc->vint = vint;
> >> +	desc->global_event = global_event;
> >> +	desc->vint_status_bit = vint_status_bit;
> >> +	desc->secondary_host = s_host;
> >> +
> >> +	hash_add(info->irqs, &irq->node, ti_sci_irq_hash(desc));
> > 
> > No locking? set_irq can be invoked in parallel paths, no?
> > Further, should'nt we check if the same src_id and src_index is already
> > present before adding to hash list?
> 
> ti_sci_manage_irq(TI_SCI_MSG_SET_IRQ) is the lock. If it succeeds we
> have to add it in hash list.

Do document it in code. it is not an obvious path.

> 
> Can set_irq() and free_irq() be invoked in parallel paths? In this case
> maybe I should add a lock for set_irq() and free_irq().

Yes - drivers are running all in parallel, right?

[...]

> >>  
> >> +	switch (pm_suspend_target_state) {
> >> +	case PM_SUSPEND_MEM:
> >> +		if (info->fw_caps & MSG_FLAG_CAPS_LPM_IRQ_CONTEXT_LOST) {
> >> +			hash_for_each_safe(info->irqs, i, tmp_node, irq, node) {
> >> +				irq_desc = &irq->desc;
> >> +				ret = ti_sci_manage_irq(&info->handle,
> >> +							irq_desc->valid_params,
> >> +							irq_desc->src_id,
> >> +							irq_desc->src_index,
> >> +							irq_desc->dst_id,
> >> +							irq_desc->dst_host_irq,
> >> +							irq_desc->ia_id,
> >> +							irq_desc->vint,
> >> +							irq_desc->global_event,
> >> +							irq_desc->vint_status_bit,
> >> +							irq_desc->secondary_host,
> >> +							TI_SCI_MSG_SET_IRQ);
> >> +				if (ret)
> >> +					return ret;
> > 
> > Do you want to attempt to restore the rest of the entries rather than give
> > up on the first fail? Maybe just log the error for debug and attempt the
> > rest?
> 
> In this case, if I get more than one error what value should I return?

Probably the first one (or the last one - i wouldn't think it matters)
- we might need to log the other errors though.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource