Andrea Parri <parri.andrea@xxxxxxxxx> writes:
>> > @@ -244,21 +234,18 @@ int vmbus_connect(void)
>> > * version.
>> > */
>> >
>> > - version = VERSION_CURRENT;
>> > + for (i = 0; ; i++) {
>> > + version = vmbus_versions[i];
>> > + if (version == VERSION_INVAL)
>> > + goto cleanup;
>>
>> If you use e.g. ARRAY_SIZE() you can get rid of VERSION_INVAL - and make
>> this code look more natural.
>
> Thank you for pointing this out, Vitaly.
>
> IIUC, you're suggesting that I do:
>
> for (i = 0; i < ARRAY_SIZE(vmbus_versions); i++) {
> version = vmbus_versions[i];
>
> ret = vmbus_negotiate_version(msginfo, version);
> if (ret == -ETIMEDOUT)
> goto cleanup;
>
> if (vmbus_connection.conn_state == CONNECTED)
> break;
> }
>
> if (vmbus_connection.conn_state != CONNECTED)
> break;
>
> and that I remove VERSION_INVAL from vmbus_versions, yes?
>
Yes, something like that.
> Looking at the uses of VERSION_INVAL, I find one remaining occurrence
> of this macro in vmbus_bus_resume(), which does:
>
> if (vmbus_proto_version == VERSION_INVAL ||
> vmbus_proto_version == 0) {
> ...
> }
>
> TBH I'm looking at vmbus_bus_resume() and vmbus_bus_suspend() for the
> first time and I'm not sure about how to change such check yet... any
> suggestions?
Hm, I don't think vmbus_proto_version can ever become == VERSION_INVAL
if we rewrite the code the way you suggest, right? So you'll reduce this
to 'if (!vmbus_proto_version)' meaning we haven't negotiated any version
(yet).
>
> Mmh, I see that vmbus_bus_resume() and vmbus_bus_suspend() can access
> vmbus_connection.conn_state: can such accesses race with a concurrent
> vmbus_connect()?
Let's summon Dexuan who's the author! :-)
>
> Thanks,
> Andrea
>
>
>> >
>> > - do {
>> > ret = vmbus_negotiate_version(msginfo, version);
>> > if (ret == -ETIMEDOUT)
>> > goto cleanup;
>> >
>> > if (vmbus_connection.conn_state == CONNECTED)
>> > break;
>> > -
>> > - version = vmbus_get_next_version(version);
>> > - } while (version != VERSION_INVAL);
>> > -
>> > - if (version == VERSION_INVAL)
>> > - goto cleanup;
>> > + }
>> >
>> > vmbus_proto_version = version;
>> > pr_info("Vmbus version:%d.%d\n",
--
Vitaly
