From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 46B2FC43381 for ; Fri, 5 Mar 2021 08:13:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1B51964E45 for ; Fri, 5 Mar 2021 08:13:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229464AbhCEINF (ORCPT ); Fri, 5 Mar 2021 03:13:05 -0500 Received: from mga12.intel.com ([192.55.52.136]:13733 "EHLO mga12.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229517AbhCEIMv (ORCPT ); Fri, 5 Mar 2021 03:12:51 -0500 IronPort-SDR: xIjIGXCsTMqPQY8M22lwwS81msckS/WRZDrwpP9oL7mUfrEIJMaB/QsxHcJZlK4O56u8o8ef/2 S2ay84/H83MQ== X-IronPort-AV: E=McAfee;i="6000,8403,9913"; a="166863983" X-IronPort-AV: E=Sophos;i="5.81,224,1610438400"; d="scan'208";a="166863983" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Mar 2021 00:12:51 -0800 IronPort-SDR: gOM/RV1MGFF6djhiCzZwd71anEmVTMnJCG2twRD5Am2DSoCo9aEygW3cGFIEUahEX4KzpyYu8z HmXiSROUba8g== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.81,224,1610438400"; d="scan'208";a="597700002" Received: from unknown (HELO [10.239.154.55]) ([10.239.154.55]) by fmsmga006.fm.intel.com with ESMTP; 05 Mar 2021 00:12:45 -0800 Subject: Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver To: Jason Wang , linux-i2c@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org Cc: mst@redhat.com, wsa@kernel.org, wsa+renesas@sang-engineering.com, andriy.shevchenko@linux.intel.com, conghui.chen@intel.com, arnd@arndb.de, kblaiech@mellanox.com, jarkko.nikula@linux.intel.com, Sergey.Semin@baikalelectronics.ru, rppt@kernel.org, loic.poulain@linaro.org, tali.perry1@gmail.com, u.kleine-koenig@pengutronix.de, bjorn.andersson@linaro.org, yu1.wang@intel.com, shuo.a.liu@intel.com, viresh.kumar@linaro.org, stefanha@redhat.com, pbonzini@redhat.com References: <9a2086f37c0a62069b67c39a3f75941b78a0039c.1614749417.git.jie.deng@intel.com> <43b0842b-8b0f-1979-ed07-d6124e3a6b79@redhat.com> <76554717-fc77-1c63-58d2-58aa04f7af9b@intel.com> <7aa23b71-01eb-5824-b1db-892ede690fce@redhat.com> From: Jie Deng Message-ID: Date: Fri, 5 Mar 2021 16:12:44 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <7aa23b71-01eb-5824-b1db-892ede690fce@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2021/3/5 15:23, Jason Wang wrote: > >>>> +    virtqueue_kick(vq); >>>> + >>>> +    time_left = wait_for_completion_timeout(&vi->completion, >>>> adap->timeout); >>>> +    if (!time_left) { >>>> +        dev_err(&adap->dev, "virtio i2c backend timeout.\n"); >>>> +        ret = -ETIMEDOUT; >>>> +        goto err_unlock_free; >>> >>> >>> So if the request is finished after the timerout, all the following >>> request will fail, is this expected? >>> >>> >> This is an expected behavior. If timeout happens, we don't need to >> care about the requests whether >> really completed by "HW" or not. Just return error and let the i2c >> core to decide whether to resend. > > > So you need at least reinit the completion at least? > Right. Will fix it. Thank you. > >>>> +    } >>>> + >>>> +    ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr); >>> >>> >>> So consider driver queue N requests, can device raise interrupt if >>> it completes the first request? If yes, the code break, if not it >>> need to be clarified in the spec. >> The device can raise interrupt when some requests are still not >> completed though this is not a good operation. > > > Then you need forbid this in the spec. > Yeah, but I think we can add some description to explain this clearly instead of forbid it directly. > >> In this case, the remaining requests in the vq will be ignored and >> the i2c_algorithm. master_xfer will return 1 for >> your example. And let the i2c core to decide whether to resend. >>> >>> Acaultly I remember there's no VIRTIO_I2C_FLAGS_FAIL_NEXT in >>> previous versions, and after reading the spec I still don't get the >>> motivation for that (it may complicates both driver and device >>> actually). >>> >> This flag is introduced by Stefan. Please check following link for >> the details >> https://lists.oasis-open.org/archives/virtio-comment/202012/msg00075.html. >> > > > > We just need to make sure that once the driver adds some requests to > the > > virtqueue, > > it should complete them (either success or fail) before adding new > requests. > > I think this > > is a behavior of physical I2C adapter drivers and we can follow. > > > Is this a driver requirement or device? If it's the driver, the code > have already did something like this. E.g you wait for the completion > of the request and forbid new request via i2c_lock. > > Thanks > The driver.  VIRTIO_I2C_FLAGS_FAIL_NEXT doesn't help in Linux driver. But I agree with Stefan that VIRTIO is not specific to Linux so the specs design should avoid the limitations of the current Linux driver behavior. > >> >