Re: [Bulk] Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
But the only effect of returning EINVAL is a printk (for this particular driver): /* PCI Memory-Write-Invalidate cycle support is optional (uncommon) */ retval = pci_set_mwi(pdev); if (!retval) ehci_dbg(ehci, MWI active\n); Erm, I've lost context here but it's completely legit for hardware to NOT support MWI, so it is in no way an error if it's not set. (Memory-Write-Invalidate is just a more efficient way to write data that may be cached; if the device can't issue those cycles, there's no loss of correctness.) Since it's not an error, there should be no such printk ... which is exactly how it's coded above. Who is issuing the printk on a non-error code path?? - Dave - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bulk] Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
On Sun, Oct 15, 2006 at 12:08:09AM -0700, David Brownell wrote: But the only effect of returning EINVAL is a printk (for this particular driver): /* PCI Memory-Write-Invalidate cycle support is optional (uncommon) */ retval = pci_set_mwi(pdev); if (!retval) ehci_dbg(ehci, MWI active\n); Erm, I've lost context here but it's completely legit for hardware to NOT support MWI, so it is in no way an error if it's not set. (Memory-Write-Invalidate is just a more efficient way to write data that may be cached; if the device can't issue those cycles, there's no loss of correctness.) Since it's not an error, there should be no such printk ... which is exactly how it's coded above. Who is issuing the printk on a non-error code path?? Er, that would be the EHCI driver, which you wrote ... - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bulk] Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
Ar Sul, 2006-10-15 am 00:08 -0700, ysgrifennodd David Brownell: Since it's not an error, there should be no such printk ... which is exactly how it's coded above. The underlying bug is that someone marked pci_set_mwi must-check, that's wrong for most of the drivers that use it. If you remove the must check annotation from it then the problem and a thousand other spurious warnings go away. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bulk] Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
On Sun, Oct 15, 2006 at 03:21:22PM +0100, Alan Cox wrote: Ar Sul, 2006-10-15 am 00:08 -0700, ysgrifennodd David Brownell: Since it's not an error, there should be no such printk ... which is exactly how it's coded above. The underlying bug is that someone marked pci_set_mwi must-check, that's wrong for most of the drivers that use it. If you remove the must check annotation from it then the problem and a thousand other spurious warnings go away. There's only about 20 users of pci_set_mwi ... about 12 of them seem to check it, one of them uses a variable called compiler_warning_pointless_fix which leaves about 7 warnings to be removed by removing the __must_check. However, I do believe the __must_check should be removed. For example, the LSI 53c1030 has *nothing* to be done if setting MWI fails. It just doesn't work, and the device copes. It's not like Tulip or sym53c8xx where there are additional bits to be set or cleared in control registers. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bulk] Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
On Sun, 15 Oct 2006 07:57:56 -0600 Matthew Wilcox [EMAIL PROTECTED] wrote: On Sun, Oct 15, 2006 at 03:21:22PM +0100, Alan Cox wrote: Ar Sul, 2006-10-15 am 00:08 -0700, ysgrifennodd David Brownell: Since it's not an error, there should be no such printk ... which is exactly how it's coded above. The underlying bug is that someone marked pci_set_mwi must-check, that's wrong for most of the drivers that use it. If you remove the must check annotation from it then the problem and a thousand other spurious warnings go away. There's only about 20 users of pci_set_mwi ... about 12 of them seem to check it, one of them uses a variable called compiler_warning_pointless_fix which leaves about 7 warnings to be removed by removing the __must_check. However, I do believe the __must_check should be removed. For example, the LSI 53c1030 has *nothing* to be done if setting MWI fails. It just doesn't work, and the device copes. If the drivers doesn't care and if it makes no difference to performance then just delete the call to pci_set_mwi(). But if MWI _does_ make a difference to performance then we should tell someone that it isn't working rather than silently misbehaving? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bulk] Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
Ar Sul, 2006-10-15 am 10:45 -0700, ysgrifennodd Andrew Morton: If the drivers doesn't care and if it makes no difference to performance then just delete the call to pci_set_mwi(). But if MWI _does_ make a difference to performance then we should tell someone that it isn't working rather than silently misbehaving? It isn't misbehaving it just isn't available. MWI is rather different to say pci_set_master() in that it makes a lot of sense for many drivers to ask for it but not care about the results. Something like pci_set_master failing is a big problem and has to be handled (although as we often don't use BIOS PCI services we see fake success in some cases). MWI is an extra cheese option not a no pizza case Alan - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bulk] Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
Andrew Morton writes: If the drivers doesn't care and if it makes no difference to performance then just delete the call to pci_set_mwi(). But if MWI _does_ make a difference to performance then we should tell someone that it isn't working rather than silently misbehaving? That sounds like we need a printk inside pci_set_mwi then, rather than adding a printk to every single caller. Paul. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bulk] Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
On Mon, 16 Oct 2006 10:00:25 +1000 Paul Mackerras [EMAIL PROTECTED] wrote: Andrew Morton writes: If the drivers doesn't care and if it makes no difference to performance then just delete the call to pci_set_mwi(). But if MWI _does_ make a difference to performance then we should tell someone that it isn't working rather than silently misbehaving? That sounds like we need a printk inside pci_set_mwi then, rather than adding a printk to every single caller. I think so, yes. That's a good solution to a lot of these things. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bulk] Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
If the drivers doesn't care and if it makes no difference to performance then just delete the call to pci_set_mwi(). But if MWI _does_ make a difference to performance then we should tell someone that it isn't working rather than silently misbehaving? To repeat: it's not misbehaving since support for MWI cycles is completely optional. It'd be more interesting to get messages in the cases that it can be enabled, since typically it can't be. That sounds like we need a printk inside pci_set_mwi then, rather than adding a printk to every single caller. Maybe wrapped in #ifdef CONFIG_SPAM_MY_KERNEL_LOG_MESSAGES ... :) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
