[pathc][v2] Fam10 name string

[ron minnich]

Acked-by: Ronald G. Minnich

So, Marc, do your hands ever get tired :-)

ron

[Peter Stuge]

On Wed, Apr 23, 2008 at 06:02:19PM -0600, Marc Jones wrote:
> Add CPUID processor name string support for Fam10 CPUs.
>
> Signed-off-by: Marc Jones

Sorry.
> +static inline void strcpy(char *dst, char *src)
> +{
> while (*src) *dst++ = *src++;
>
> +}

This isn't somewhere already?
> +static const struct {
> u8 Pg;
> u8 NC;
> u8 String1;
> char const *value;
>
> +} String1_socket_F[] = {

I'd use the same struct for all tables..
> if (Model > 0) {
> /* String1 */
> if (PkgTyp == 0) {		/* F1207 */
> 	for(i = 0; i < sizeof(String1_socket_F)/sizeof(String1_socket_F[0]); i++) {
> 		if ((String1_socket_F[i].Pg == Pg) &&
> 		    (String1_socket_F[i].NC == NC) &&
> 		    (String1_socket_F[i].String1 == String1)) {
> 			processor_name_string = String1_socket_F[i].value;
> 		}
> 	}
> } else if (PkgTyp == 1) {	/* AM2 */
> 	for(i = 0; i < sizeof(String1_socket_AM2)/sizeof(String1_socket_AM2[0]); i++) {
> 		if ((String1_socket_AM2[i].Pg == Pg) &&
> 		    (String1_socket_AM2[i].NC == NC) &&
> 		    (String1_socket_AM2[i].String1 == String1)) {
> 			processor_name_string = String1_socket_AM2[i].value;
> 		}
> 	}
> }

..and set a pointer to the appropriate struct, and have a single loop..
> if (processor_name_string) {
> 	strcpy(program_string, processor_name_string);
> 	/* Translate Model from 01-99 to ASCII and put it on the end.
> 	 * Numbers less than 10 should include a leading zero, e.g., 09.*/
> 	for (j = 0; j < 47; j++) {
> 		if(program_string[j] == 0) {
> 			program_string[j++] = (Model / 10) + '0';
> 			program_string[j++] = (Model % 10) + '0';

This code can produce strings that make the BIOS fail validation,
there will be one byte of buffer overrun for 47 byte long strings,
and it does not add a terminating 0 byte after the two digits, which
means also 46 byte long strings cause failure.

At least change the for loop condition to j < 45.
> 	/* String 2 */
> 	processor_name_string = NULL;
> 	if (PkgTyp == 0) {		/* F1207 */
> 		for(i = 0; i < sizeof(String2_socket_F)/sizeof(String2_socket_F[0]); i++) {
> 			if ((String2_socket_F[i].Pg == Pg) &&
> 			    (String2_socket_F[i].String2 == String2)) {
> 				processor_name_string = String2_socket_F[i].value;
> 			}
> 		}
> 	} else if (PkgTyp == 1) {	/* AM2 */
> 		for(i = 0; i < sizeof(String2_socket_AM2)/sizeof(String2_socket_AM2[0]); i++) {
> 			if ((String2_socket_AM2[i].Pg == Pg) &&
> 			    (String2_socket_AM2[i].NC == NC) &&
> 			    (String2_socket_AM2[i].String2 == String2)) {
> 				processor_name_string = String2_socket_AM2[i].value;
> 			}
> 		}
> 	}

again nicer to just pick a pointer and have a single loop..
> 	if (processor_name_string) {

that way no if is needed in either place..
> 		/* FIXME: check length but it should never happen  */
> 		strcpy(&program_string[j], processor_name_string);

..or simply always memcpy 48 bytes and clear the last byte.

//Peter

[Peter Stuge]

On Wed, Apr 23, 2008 at 05:29:25PM -0700, ron minnich wrote:
> Acked-by: Ronald G. Minnich

Too fast Ron, sorry. :\ That code was not so good.

//Peter

[Peter Stuge]

On Wed, Apr 23, 2008 at 06:02:19PM -0600, Marc Jones wrote:
> Add CPUID processor name string support for Fam10 CPUs.
>
> Signed-off-by: Marc Jones

Nak. I found more issues, bits making up Model were not masked
correctly, along with previously mentioned problems.

Attached is my counterproposal for the patch.

//Peter

[Marc Jones]

Peter Stuge wrote:
> On Wed, Apr 23, 2008 at 06:02:19PM -0600, Marc Jones wrote:
> > Add CPUID processor name string support for Fam10 CPUs.
> >
> > Signed-off-by: Marc Jones
>
> Nak. I found more issues, bits making up Model were not masked
> correctly, along with previously mentioned problems.
>
> Attached is my counterproposal for the patch.
>
> //Peter
>
> Add CPUID processor name string support for Fam10 CPUs.
>
> Signed-off-by: Marc Jones 
> Signed-off-by: Peter Stuge

Peter,

Thanks, This is much cleaner. Tested on my system.
Acked-by: Marc Jones

Marc

[Marc Jones]

Marc Jones wrote:
> Peter Stuge wrote:
> > On Wed, Apr 23, 2008 at 06:02:19PM -0600, Marc Jones wrote:
> > > Add CPUID processor name string support for Fam10 CPUs.
> > >
> > > Signed-off-by: Marc Jones
> >
> > Nak. I found more issues, bits making up Model were not masked
> > correctly, along with previously mentioned problems.
> >
> > Attached is my counterproposal for the patch.
> >
> > //Peter
> >
> > Add CPUID processor name string support for Fam10 CPUs.
> >
> > Signed-off-by: Marc Jones 
> > Signed-off-by: Peter Stuge
>
> Peter,
>
> Thanks, This is much cleaner. Tested on my system.
> Acked-by: Marc Jones
>
> Marc

r3263

Thanks,
Marc

[Peter Stuge]

On Thu, Apr 24, 2008 at 09:53:00AM -0600, Marc Jones wrote:
> > Attached is my counterproposal for the patch.
>
> Thanks, This is much cleaner. Tested on my system.
> Acked-by: Marc Jones

Great! Thanks for testing.

//Peter