Re: [PATCH] ofnet: Do not set SUFFIX for sun4v network devices

[Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>]

Hi.

On 08/08/2015 11:47 AM, Andrei Borzenkov wrote:
> В Thu, 16 Jul 2015 10:21:41 +0300
> Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com> пишет:
>
>> sun4v vnet devices do not implement the support of duplex and speed
>> instance attributes. An attempt to open such a device with
>> the attributes will fail:
>>
>> ok select net:speed=auto,duplex=auto
>> Unknown key 'speed'
>> Unknown key 'duplex'
>> Manual Configuration: Host IP, boot server and filename must be specified
>> WARNING: /virtual-devices@100/channel-devices@200/network@0: Can't open OBP standard TFTP package
>>
>> Can't open device
>> ok
>>
>> Therefore, let's not set SUFFIX for such devices.
>>
>> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
>> ---
>>   grub-core/net/drivers/ieee1275/ofnet.c |   23 +++++++++++++++++++++--
>>   1 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
>> index eea8e71..dbcf337 100644
>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>> @@ -305,6 +305,7 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
>>     grub_uint64_t prop;
>>     grub_uint8_t *pprop;
>>     char *shortname;
>> +  char need_suffix = 1;
>>
>>     if (grub_strcmp (alias->type, "network") != 0)
>>       return 0;
>> @@ -325,7 +326,25 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
>>
>>   #define SUFFIX ":speed=auto,duplex=auto,1.1.1.1,dummy,1.1.1.1,1.1.1.1,5,5,1.1.1.1,512"
>>
>> -  if (!grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_NO_OFNET_SUFFIX))
>> +  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_NO_OFNET_SUFFIX))
>> +    need_suffix = 0;
>> +
>> +  /* sun4v vnet devices do not support setting duplex/speed */
>> +  {
>> +    char tmp[24];
>> +
>> +    grub_ieee1275_finddevice (alias->path, &devhandle);
>> +
>> +    if (!grub_ieee1275_get_property (devhandle, "compatible",
>> +				     (grub_uint8_t *)tmp, sizeof(tmp), NULL))
>> +      {
>> +	if (!grub_strncmp (tmp, "SUNW,sun4v-network",
>> +			   sizeof("SUNW,sun4v-network")))
>
> Is grub_strncmp intentional? Can it have some suffix that we want to
> ignore here?

My point was to match "SUNW,sun4v-network" exactly.

>
> "compatible" property is defined as array of strings. Can it contain
> more than one value here?

In accordance to the Virtual Machine Specification,  sections 8.23.4.2 
and 8.23.4.3:

https://kenai.com/downloads/hypervisor/Hypervisor-api-2.0.pdf

it contains single value - "SUNW,sun4v-network".

But yes, indeed, "compatible" is an array of strings, and may contain 
multiple values here (at least in the future, of one decides to update 
vnet specs). I missed that.

So on I'm fixing my patch to tread "compatible" as an array, and will 
send an updated version in a moment.

Thank you for review.

>
>> +	  need_suffix = 0;
>> +      }
>> +  }
>> +
>> +  if (need_suffix)
>>       ofdata->path = grub_malloc (grub_strlen (alias->path) + sizeof (SUFFIX));
>>     else
>>       ofdata->path = grub_malloc (grub_strlen (alias->path) + 1);
>> @@ -335,7 +354,7 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
>>         return 0;
>>       }
>>     ofdata->suffix = grub_stpcpy (ofdata->path, alias->path);
>> -  if (!grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_NO_OFNET_SUFFIX))
>> +  if (need_suffix)
>>       grub_memcpy (ofdata->suffix, SUFFIX, sizeof (SUFFIX));
>>     else
>>       *ofdata->suffix = '\0';
>