On Fri, Sep 28, 2012 at 10:45:15AM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Sep 28, 2012 at 02:23:59PM +0200, Joerg Roedel wrote:
> > + /*
> > + * Mask level triggered irqs.
> > + * Use IRQ_DELAYED_DISABLE for edge triggered irqs.
>
> so how come it is not set?
Don't understand this. What is not set?
> > + if (!config_enabled(CONFIG_SMP))
> > + return -1;
>
> -1? -ENOx something?
No, -1 for compatibiblity with the other implementation of this.
> > + err = apic->cpu_mask_to_apicid_and(cfg->domain, mask, &dest);
> > + if (err) {
> > + if (assign_irq_vector(irq, cfg, data->affinity))
> > + pr_err("AMD-Vi: Failed to recover vector for irq %d\n", irq);
>
> If we do OK with the assignment of the vector, should we just continue
> on instead of returning error?
The purpose of this is to recover to the old affinity if we failed at
this point. Even in this case, setting the new affinity still failed and
we need to inform the caller about this. Thus returning the error.
Also note the pr_err message which also tells you about the
recovery-case :)
Joerg
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
On Mon, Oct 01, 2012 at 10:40:51AM +0200, Joerg Roedel wrote:
> On Fri, Sep 28, 2012 at 10:45:15AM -0400, Konrad Rzeszutek Wilk wrote:
> > On Fri, Sep 28, 2012 at 02:23:59PM +0200, Joerg Roedel wrote:
> > > + /*
> > > + * Mask level triggered irqs.
> > > + * Use IRQ_DELAYED_DISABLE for edge triggered irqs.
> >
> > so how come it is not set?
>
> Don't understand this. What is not set?
the IRQ_DELAYED_DISABLE. But I could not even find that enum anymore?
Has that been obsoleted?
>
> > > + if (!config_enabled(CONFIG_SMP))
> > > + return -1;
> >
> > -1? -ENOx something?
>
> No, -1 for compatibiblity with the other implementation of this.
OK.
>
> > > + err = apic->cpu_mask_to_apicid_and(cfg->domain, mask, &dest);
> > > + if (err) {
> > > + if (assign_irq_vector(irq, cfg, data->affinity))
> > > + pr_err("AMD-Vi: Failed to recover vector for irq %d\n", irq);
> >
> > If we do OK with the assignment of the vector, should we just continue
> > on instead of returning error?
>
> The purpose of this is to recover to the old affinity if we failed at
> this point. Even in this case, setting the new affinity still failed and
> we need to inform the caller about this. Thus returning the error.
> Also note the pr_err message which also tells you about the
> recovery-case :)
I was thinking the other way around. You recover the old affinity. Should
you print out a warning mentioning to the system admin that you could not
set the new affinity but reverted to the old one? Or will that not serve
anything except spam the kernel logs?
On Mon, Oct 01, 2012 at 09:47:53AM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 01, 2012 at 10:40:51AM +0200, Joerg Roedel wrote:
> > Don't understand this. What is not set?
>
> the IRQ_DELAYED_DISABLE. But I could not even find that enum anymore?
> Has that been obsoleted?
You are right, its was removed. Ok, I will remove that line of the
comment too.
> I was thinking the other way around. You recover the old affinity. Should
> you print out a warning mentioning to the system admin that you could not
> set the new affinity but reverted to the old one? Or will that not serve
> anything except spam the kernel logs?
Well, I don't think it makes sense to print an additional error at this
point. The error will be propagated upwards in the call-chain and in the
end it is up to the initiator how to handle this situation (for example
just return a failure to user-space).
Joerg
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
>> I was thinking the other way around. You recover the old affinity. Should
>> you print out a warning mentioning to the system admin that you could not
>> set the new affinity but reverted to the old one? Or will that not serve
>> anything except spam the kernel logs?
>
> Well, I don't think it makes sense to print an additional error at this
> point. The error will be propagated upwards in the call-chain and in the
> end it is up to the initiator how to handle this situation (for example
> just return a failure to user-space).
OK. Thx for the explanation. I was not sure whether the error would
propagate properly up the chain.