Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Andrea Parri <parri.andrea@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
        "K . Y . Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        Sasha Levin <sashal@kernel.org>,
        Michael Kelley <mikelley@microsoft.com>,
        Dexuan Cui <decui@microsoft.com>
Subject: Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
In-Reply-To: <20191008124052.GA11245@andrea.guest.corp.microsoft.com>
References: <20191007163115.26197-1-parri.andrea@gmail.com> <20191007163115.26197-2-parri.andrea@gmail.com> <87eezo1nrr.fsf@vitty.brq.redhat.com> <20191008124052.GA11245@andrea.guest.corp.microsoft.com>
Date:   Tue, 08 Oct 2019 15:00:25 +0200
Message-ID: <87zhibz91y.fsf@vitty.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=WINDOWS-1252
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Andrea Parri <parri.andrea@gmail.com> writes:

>> > @@ -244,21 +234,18 @@ int vmbus_connect(void)
>> >  =09 * version.
>> >  =09 */
>> > =20
>> > -=09version =3D VERSION_CURRENT;
>> > +=09for (i =3D 0; ; i++) {
>> > +=09=09version =3D vmbus_versions[i];
>> > +=09=09if (version =3D=3D VERSION_INVAL)
>> > +=09=09=09goto cleanup;
>>=20
>> 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:
>
> =09for (i =3D 0; i < ARRAY_SIZE(vmbus_versions); i++) {
> =09=09version =3D vmbus_versions[i];
>
> =09=09ret =3D vmbus_negotiate_version(msginfo, version);
> =09=09if (ret =3D=3D -ETIMEDOUT)
> =09=09=09goto cleanup;
>
> =09=09if (vmbus_connection.conn_state =3D=3D CONNECTED)
> =09=09=09break;
> =09}
>
> =09if (vmbus_connection.conn_state !=3D CONNECTED)
> =09=09break;
>
> 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:
>
> =09if (vmbus_proto_version =3D=3D VERSION_INVAL ||
> =09    vmbus_proto_version =3D=3D 0) {
> =09=09...
> =09}
>
> 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 =3D=3D 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! :-)=20

>
> Thanks,
>   Andrea
>
>
>> > =20
>> > -=09do {
>> >  =09=09ret =3D vmbus_negotiate_version(msginfo, version);
>> >  =09=09if (ret =3D=3D -ETIMEDOUT)
>> >  =09=09=09goto cleanup;
>> > =20
>> >  =09=09if (vmbus_connection.conn_state =3D=3D CONNECTED)
>> >  =09=09=09break;
>> > -
>> > -=09=09version =3D vmbus_get_next_version(version);
>> > -=09} while (version !=3D VERSION_INVAL);
>> > -
>> > -=09if (version =3D=3D VERSION_INVAL)
>> > -=09=09goto cleanup;
>> > +=09}
>> > =20
>> >  =09vmbus_proto_version =3D version;
>> >  =09pr_info("Vmbus version:%d.%d\n",

--=20
Vitaly