[PATCH] Loongson: Lemote-2F: Fixup of _rdmsr and _wrmsr

[PATCH] Loongson: Lemote-2F: Fixup of _rdmsr and _wrmsr

[Wu Zhangjin]

From: Wu Zhangjin <wuzhangjin@gmail.com>

The _rdmsr, _wrmsr operation must be atomic to ensure the accessing to msr
address is what we want.

Without this patch, in some cases, the reboot operation(fs2f_reboot) defined in
arch/mips/loongson/lemote-2f/reset.c may fail for it called _rdmsr, _wrmsr but
may be interrupted/preempted by the other related operations and make the
_rdmsr get the wrong value or make the _wrmsr write a wrong value to an
unexpected target.

Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
---
 arch/mips/pci/ops-loongson2.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/mips/pci/ops-loongson2.c b/arch/mips/pci/ops-loongson2.c
index 2bb4057..1f93dfb 100644
--- a/arch/mips/pci/ops-loongson2.c
+++ b/arch/mips/pci/ops-loongson2.c
@@ -180,15 +180,20 @@ struct pci_ops loongson_pci_ops = {
 };
 
 #ifdef CONFIG_CS5536
+DEFINE_SPINLOCK(msr_lock);
 void _rdmsr(u32 msr, u32 *hi, u32 *lo)
 {
 	struct pci_bus bus = {
 		.number = PCI_BUS_CS5536
 	};
 	u32 devfn = PCI_DEVFN(PCI_IDSEL_CS5536, 0);
+	unsigned long flags;
+
+	spin_lock_irqsave(&msr_lock, flags);
 	loongson_pcibios_write(&bus, devfn, PCI_MSR_ADDR, 4, msr);
 	loongson_pcibios_read(&bus, devfn, PCI_MSR_DATA_LO, 4, lo);
 	loongson_pcibios_read(&bus, devfn, PCI_MSR_DATA_HI, 4, hi);
+	spin_unlock_irqrestore(&msr_lock, flags);
 }
 EXPORT_SYMBOL(_rdmsr);
 
@@ -198,9 +203,13 @@ void _wrmsr(u32 msr, u32 hi, u32 lo)
 		.number = PCI_BUS_CS5536
 	};
 	u32 devfn = PCI_DEVFN(PCI_IDSEL_CS5536, 0);
+	unsigned long flags;
+
+	spin_lock_irqsave(&msr_lock, flags);
 	loongson_pcibios_write(&bus, devfn, PCI_MSR_ADDR, 4, msr);
 	loongson_pcibios_write(&bus, devfn, PCI_MSR_DATA_LO, 4, lo);
 	loongson_pcibios_write(&bus, devfn, PCI_MSR_DATA_HI, 4, hi);
+	spin_unlock_irqrestore(&msr_lock, flags);
 }
 EXPORT_SYMBOL(_wrmsr);
 #endif
-- 
1.7.0.1

Re: [PATCH] Loongson: Lemote-2F: Fixup of _rdmsr and _wrmsr

[David Daney]

On 03/10/2010 07:41 AM, Wu Zhangjin wrote:
> From: Wu Zhangjin<wuzhangjin@gmail.com>
>
> The _rdmsr, _wrmsr operation must be atomic to ensure the accessing to msr
> address is what we want.
>
> Without this patch, in some cases, the reboot operation(fs2f_reboot) defined in
> arch/mips/loongson/lemote-2f/reset.c may fail for it called _rdmsr, _wrmsr but
> may be interrupted/preempted by the other related operations and make the
> _rdmsr get the wrong value or make the _wrmsr write a wrong value to an
> unexpected target.
>
> Signed-off-by: Wu Zhangjin<wuzhangjin@gmail.com>
> ---
>   arch/mips/pci/ops-loongson2.c |    9 +++++++++
>   1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/pci/ops-loongson2.c b/arch/mips/pci/ops-loongson2.c
> index 2bb4057..1f93dfb 100644
> --- a/arch/mips/pci/ops-loongson2.c
> +++ b/arch/mips/pci/ops-loongson2.c
> @@ -180,15 +180,20 @@ struct pci_ops loongson_pci_ops = {
>   };
>
>   #ifdef CONFIG_CS5536
> +DEFINE_SPINLOCK(msr_lock);


Should this be DEFINE_RAW_SPINLOCK instead?

David Daney


>   void _rdmsr(u32 msr, u32 *hi, u32 *lo)
>   {
>   	struct pci_bus bus = {
>   		.number = PCI_BUS_CS5536
>   	};
>   	u32 devfn = PCI_DEVFN(PCI_IDSEL_CS5536, 0);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&msr_lock, flags);
>   	loongson_pcibios_write(&bus, devfn, PCI_MSR_ADDR, 4, msr);
>   	loongson_pcibios_read(&bus, devfn, PCI_MSR_DATA_LO, 4, lo);
>   	loongson_pcibios_read(&bus, devfn, PCI_MSR_DATA_HI, 4, hi);
> +	spin_unlock_irqrestore(&msr_lock, flags);
>   }
>   EXPORT_SYMBOL(_rdmsr);
>
> @@ -198,9 +203,13 @@ void _wrmsr(u32 msr, u32 hi, u32 lo)
>   		.number = PCI_BUS_CS5536
>   	};
>   	u32 devfn = PCI_DEVFN(PCI_IDSEL_CS5536, 0);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&msr_lock, flags);
>   	loongson_pcibios_write(&bus, devfn, PCI_MSR_ADDR, 4, msr);
>   	loongson_pcibios_write(&bus, devfn, PCI_MSR_DATA_LO, 4, lo);
>   	loongson_pcibios_write(&bus, devfn, PCI_MSR_DATA_HI, 4, hi);
> +	spin_unlock_irqrestore(&msr_lock, flags);
>   }
>   EXPORT_SYMBOL(_wrmsr);
>   #endif

Re: [PATCH] Loongson: Lemote-2F: Fixup of _rdmsr and _wrmsr

[Wu Zhangjin]

On Wed, 2010-03-10 at 16:17 -0800, David Daney wrote:
> On 03/10/2010 07:41 AM, Wu Zhangjin wrote:
> > From: Wu Zhangjin<wuzhangjin@gmail.com>
> >
> > The _rdmsr, _wrmsr operation must be atomic to ensure the accessing to msr
> > address is what we want.
> >
> > Without this patch, in some cases, the reboot operation(fs2f_reboot) defined in
> > arch/mips/loongson/lemote-2f/reset.c may fail for it called _rdmsr, _wrmsr but
> > may be interrupted/preempted by the other related operations and make the
> > _rdmsr get the wrong value or make the _wrmsr write a wrong value to an
> > unexpected target.
> >
> > Signed-off-by: Wu Zhangjin<wuzhangjin@gmail.com>
> > ---
> >   arch/mips/pci/ops-loongson2.c |    9 +++++++++
> >   1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/mips/pci/ops-loongson2.c b/arch/mips/pci/ops-loongson2.c
> > index 2bb4057..1f93dfb 100644
> > --- a/arch/mips/pci/ops-loongson2.c
> > +++ b/arch/mips/pci/ops-loongson2.c
> > @@ -180,15 +180,20 @@ struct pci_ops loongson_pci_ops = {
> >   };
> >
> >   #ifdef CONFIG_CS5536
> > +DEFINE_SPINLOCK(msr_lock);
> 
> 
> Should this be DEFINE_RAW_SPINLOCK instead?
> 

It should be, thanks!

Regards,
	Wu Zhangjin