Re: [PATCH v2] sparc: Resolve conflict between sparc v9 and M7 on usage of bit 9 of TTE

[Khalid Aziz]

On 05/24/2015 02:00 PM, David Miller wrote:
> From: Khalid Aziz <khalid.aziz@xxxxxxxxxx>
> Date: Fri, 15 May 2015 11:48:15 -0600
>
> > diff --git a/arch/sparc/kernel/setup_64.c b/arch/sparc/kernel/setup_64.c
> > index c38d19f..606e3f9 100644
> > --- a/arch/sparc/kernel/setup_64.c
> > +++ b/arch/sparc/kernel/setup_64.c
> > @@ -255,6 +255,30 @@ void sun4v_patch_2insn_range(struct sun4v_2insn_patch_entry *start,
> >   	}
> >   }
> >
> > +void sun_m7_patch_2insn_range(struct sun4v_2insn_patch_entry *start,
> > +			     struct sun4v_2insn_patch_entry *end)
> > +{
> > +	while (start < end) {
> > +		unsigned long addr = start->addr;
> > +
> > +		*(unsigned int *) (addr +  0) = start->insns[0];
> > +		/* We are updating an instruction. Make sure it is
> > +		 * written out
> > +		 */
> > +		wmb();
> > +		__asm__ __volatile__("flush	%0" : : "r" (addr +  0));
> > +
> > +		*(unsigned int *) (addr +  4) = start->insns[1];
> > +		/* We are updating an instruction. Make sure it is
> > +		 * written out
> > +		 */
> > +		wmb();
> > +		__asm__ __volatile__("flush	%0" : : "r" (addr +  4));
>
> There is no reason to say this in a comment, none of the other code
> patching routines mention this, and it's even more excessive to say it
> twice in quick succession basically in the same place.
>
> Please just remove these comments, anyone reading this code knows we
> are code patching and will realize that there are memory barrier
> requirements.
>
> Alternatively if you think it's worth mentioning, submit a separate
> patch that adds the comment to all of the code patching routines for
> consistency.
>
> > @@ -267,6 +291,9 @@ static void __init sun4v_patch(void)
> >
> >   	sun4v_patch_2insn_range(&__sun4v_2insn_patch,
> >   				&__sun4v_2insn_patch_end);
> > +	if (sun4v_chip_type == SUN4V_CHIP_SPARC_M7)
> > +		sun_m7_patch_2insn_range(&__sun_m7_2insn_patch,
> > +				&__sun_m7_2insn_patch_end);
>
> When a function call spans multiple lines, the second and subsequent line
> must start at exactly the first column after the openning parenthesis of
> the first line.  You must use the appropriate number of TAB and SPACE
> characters necessary to achieve this.
>
> Look at how the sun4v_patch_2insn_range() call before the one you added is
> indented.
>
> > @@ -2312,8 +2345,7 @@ int __meminit vmemmap_populate(unsigned long vstart, unsigned long vend,
> >   		    _PAGE_P_4U | _PAGE_W_4U);
> >   	if (tlb_type == hypervisor)
> >   		pte_base = (_PAGE_VALID | _PAGE_SZ4MB_4V |
> > -			    _PAGE_CP_4V | _PAGE_CV_4V |
> > -			    _PAGE_P_4V | _PAGE_W_4V);
> > +			page_cache4v_flag | _PAGE_P_4V | _PAGE_W_4V);
>
> The existing indentation of these lines after the first of the
> pte_base assignment were correct, please do not change how it is
> indented.
>
> > @@ -2465,8 +2497,8 @@ static void __init sun4v_pgprot_init(void)
> >   	kern_linear_pte_xor[0] = (_PAGE_VALID | _PAGE_SZ4MB_4V) ^
> >   		PAGE_OFFSET;
> >   #endif
> > -	kern_linear_pte_xor[0] |= (_PAGE_CP_4V | _PAGE_CV_4V |
> > -				   _PAGE_P_4V | _PAGE_W_4V);
> > +	kern_linear_pte_xor[0] |= (page_cache4v_flag | _PAGE_P_4V |
> > +				_PAGE_W_4V);
>
> Likewise.

All of these changes were caused by my addressing checkpatch barfing, 
but the way you suggest indenting is more readable. I will clean all 
this up, ignore checkpatch, test the updated patch and send a new version.

Thanks,
Khalid

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/