Re: [PATCH 10/21] MSI: Add an arch_msi_supported()

Re: [PATCH 10/21] MSI: Add an arch_msi_supported()

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

> Add an arch_msi_supported(), which gives archs a chance to check the input
> to pci_enable_msi/x. For MSI-X this routine might need the entry array, so
> pass it in. For plain MSI, NULL is passed, the arch routine needs to cope
> with that. Propagate the error value returned from the arch routine out to
> the caller.

Ugh.  I'm not very comfortable with passing struct msix_entry into
the architectures right now.

There are a couple of reasons.
- It's irq field is to small (so we need to change it at some point)
- No a single driver that calls pci_enable_msix uses the scatter gather
  feature (so the entry member is redundant).

So this struct msix_entry needs to change and we need to change the drivers
along with it.  Having to change a couple of architectures as well sounds
painful.  So we might as well fix that at the same time as we are
adding the RTAS support so architectures don't have to deal with this
nasty unused concept.

I'm thinking the same thing to do is to completely remove struct msix_entry
and just let drivers walk the linked list you introduce a few patches
later down.  All they need is to get their irq numbers anyway.

I was tempted to drop nvec as well since our irq numbers are virtual,
we could always delay the failure into request_irq.  But there are
a few embedded architectures like the arm where the number irqs
numbers may stay limited for a long time and if the driver will never
use all of the irqs we get to save some resources and some work.  So
that makes sense.

So can we please at least move this patch down to the end with the
rest of the RTAS arch support?

Moving it towards the end will allow it to be reviewed in the context
where it will be used and it will give us a chance to simplify
pci_enable_msix before we get there.

Eric

Re: [PATCH 10/21] MSI: Add an arch_msi_supported()

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]

On Tue, 2007-03-27 at 23:54 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
> 
> > Add an arch_msi_supported(), which gives archs a chance to check the input
> > to pci_enable_msi/x. For MSI-X this routine might need the entry array, so
> > pass it in. For plain MSI, NULL is passed, the arch routine needs to cope
> > with that. Propagate the error value returned from the arch routine out to
> > the caller.
> 
> Ugh.  I'm not very comfortable with passing struct msix_entry into
> the architectures right now.
> 
> There are a couple of reasons.
> - It's irq field is to small (so we need to change it at some point)
> - No a single driver that calls pci_enable_msix uses the scatter gather
>   feature (so the entry member is redundant).
> 
> So this struct msix_entry needs to change and we need to change the drivers
> along with it.  Having to change a couple of architectures as well sounds
> painful.  So we might as well fix that at the same time as we are
> adding the RTAS support so architectures don't have to deal with this
> nasty unused concept.
> 
> I'm thinking the same thing to do is to completely remove struct msix_entry
> and just let drivers walk the linked list you introduce a few patches
> later down.  All they need is to get their irq numbers anyway.

I agree with most of that. I thought of doing that change, but didn't
want to have the powerpc code stuck behind a huge pile of driver
changes.

My only other worry is that at some point we'll get a driver that does
want to choose the entries it's allocated, and at that point we'll have
to put back the msix_entry code (or something similar). I don't have any
idea of when/if that sort of hardware/driver requirement is likely to
surface though, if it's "not for a while" it might be worth ripping out
the complexity until we really need it.

> I was tempted to drop nvec as well since our irq numbers are virtual,
> we could always delay the failure into request_irq.  But there are
> a few embedded architectures like the arm where the number irqs
> numbers may stay limited for a long time and if the driver will never
> use all of the irqs we get to save some resources and some work.  So
> that makes sense.

I think nvec should stay.

> So can we please at least move this patch down to the end with the
> rest of the RTAS arch support?
> 
> Moving it towards the end will allow it to be reviewed in the context
> where it will be used and it will give us a chance to simplify
> pci_enable_msix before we get there.

I'm happy to move it to the end of the series. I'm also happy to stop
passing the msix_entry into the arch.

But I don't want to predicate the merge of our powerpc stuff on the
removal of msix_entry entirely, there's too much risk that we'll slip to
v23.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 10/21] MSI: Add an arch_msi_supported()

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

> I agree with most of that. I thought of doing that change, but didn't
> want to have the powerpc code stuck behind a huge pile of driver
> changes.
>
> My only other worry is that at some point we'll get a driver that does
> want to choose the entries it's allocated, and at that point we'll have
> to put back the msix_entry code (or something similar). I don't have any
> idea of when/if that sort of hardware/driver requirement is likely to
> surface though, if it's "not for a while" it might be worth ripping out
> the complexity until we really need it.

Yes.

Allocating everything and just requesting the irqs you really want is
works as well.  So drivers like that would need to be common and the
savings significant before it would really be worthwhile to change
the API back the way it is now.

>> I was tempted to drop nvec as well since our irq numbers are virtual,
>> we could always delay the failure into request_irq.  But there are
>> a few embedded architectures like the arm where the number irqs
>> numbers may stay limited for a long time and if the driver will never
>> use all of the irqs we get to save some resources and some work.  So
>> that makes sense.
>
> I think nvec should stay.

Agreed.

>> So can we please at least move this patch down to the end with the
>> rest of the RTAS arch support?
>> 
>> Moving it towards the end will allow it to be reviewed in the context
>> where it will be used and it will give us a chance to simplify
>> pci_enable_msix before we get there.
>
> I'm happy to move it to the end of the series. I'm also happy to stop
> passing the msix_entry into the arch.
>
> But I don't want to predicate the merge of our powerpc stuff on the
> removal of msix_entry entirely, there's too much risk that we'll slip to
> v23.

Sure.   But if we can kill msix_entry in the same time frame it would
be a good thing.

Eric