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.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 34443C48BD1 for ; Thu, 10 Jun 2021 16:58:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1E1AC613DD for ; Thu, 10 Jun 2021 16:58:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231720AbhFJRAO (ORCPT ); Thu, 10 Jun 2021 13:00:14 -0400 Received: from mail-il1-f179.google.com ([209.85.166.179]:37496 "EHLO mail-il1-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231672AbhFJRAN (ORCPT ); Thu, 10 Jun 2021 13:00:13 -0400 Received: by mail-il1-f179.google.com with SMTP id j26so2524327ila.4 for ; Thu, 10 Jun 2021 09:58:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0X69O8OJs898lKTihB59RN3w4SBwx9KHHnsAenVewI4=; b=MrDWnNTX6UU0dgPyVnPoaAwTdlrdSYBcFnMHsFmtnT5nmCLJZBqoxDdFl00SFduCeW fqk+vlydPeX9YBL3b+ulXL23uiuGPCVZtV4S90UCLscus1N0mm/jxeR7MHbyBQ8E2QxD jF0Nj0hhQ6HFsy7QrKDV1SgUHCWUwGWVfotwBtADyV+QHZwPkuCwTqq+HD83OYgrQOBn zjKzIMbJ3IxW903rDc/kU8TK49lWRCQ8dK9BYL6hqFqRmOHldjMuDcoHRfbqyE3lKzSs HO8YOIsWMkwxsUdX1KkZK00lqMvHCe3seWkcDIWMsEmskwgwO+tDyZkAh4pL0mG6fA2J 1k6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0X69O8OJs898lKTihB59RN3w4SBwx9KHHnsAenVewI4=; b=QfyG6xItRO+zd2fMcrrJqea3BLIZsULFn7Ey6RzyTc7qEwLoHVOGUpy/uxVWgCoKwY CtRTYYdTLb9a3wYzcp6VOUcMdycSJfbQqxN1Gd80Fin15YHXEk+ZMxn167jbicRBdHZC pLwHmWDrYdLs7QCG5rSlkSeITp/rY6FniMYxOqfsDeCwXz2mvld4lvzyjbjcvsB/DzuW ohHX7817oeOxvx286nuLDr7l04E+PSpxx8OryyTUsDT/4gFlxucqyi0YCPSPeZ+vNAu8 nkpPFRNoXx3OgOlv4y5PPFWeLRuBxBXcaDpKO+hhfI5+b0wjPkCi79+Jkuc+8MqHZuhu O1IA== X-Gm-Message-State: AOAM530Cd6qHWyanKvjLshTr4PGWxBghkkLe95sryTWuySaqU5ATtsZM +uZEYsrzquDrmgC6k3AFfXbHHlGdtN9TviVpLxry3Ubd99Q5fw== X-Google-Smtp-Source: ABdhPJzvvpd9vpgMr61oKKUPqSKUbzq+CFCSV14x8+buD6sK508dF1frWVtuvrj42uvJsInWXupzdjC/cqHeREHAuvc= X-Received: by 2002:a92:7607:: with SMTP id r7mr4800102ilc.31.1623344236838; Thu, 10 Jun 2021 09:57:16 -0700 (PDT) MIME-Version: 1.0 References: <10442926ae8a65f716bfc23f32339a6b35e51d5a.1623326176.git.viresh.kumar@linaro.org> <96994f4c-8755-90a8-0c50-4e21c436f137@metux.net> In-Reply-To: <96994f4c-8755-90a8-0c50-4e21c436f137@metux.net> From: Viresh Kumar Date: Thu, 10 Jun 2021 22:27:05 +0530 Message-ID: Subject: Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver To: "Enrico Weigelt, metux IT consult" Cc: Bartosz Golaszewski , Linus Walleij , "Enrico Weigelt, metux IT consult" , Viresh Kumar , "Michael S. Tsirkin" , Jason Wang , Vincent Guittot , Bill Mills , =?UTF-8?B?QWxleCBCZW5uw6ll?= , Stratos Mailing List , "linux-gpio@vger.kernel.org" , Linux Kernel Mailing List , Stefan Hajnoczi , "Stefano Garzarella --cc virtualization @ lists . linux-foundation . org" , virtualization@lists.linux-foundation.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Enrico, On Thu, 10 Jun 2021 at 21:24, Enrico Weigelt, metux IT consult wrote: > On 10.06.21 14:16, Viresh Kumar wrote: > > From: "Enrico Weigelt, metux IT consult" > > > > This patch adds a new driver for Virtio based GPIO devices. > > > > This allows a guest VM running Linux to access GPIO device provided by > > the host. It supports all basic operations for now, except interrupts > > for the GPIO lines. > > What exactly did you change here from my original driver ? I didn't write it from scratch and used your patch only to start with, and so you are still the Author of this particular patch. This just followed the specification updates and so changes only the stuff that was different from your original specs. Apart from that as you know, the irqs weren't working in your case as they needed to be implemented differently (patch 2 does that) here. > Your already changed the spec havily (w/o consulting me first), Isn't that what we are doing on the list? I believe that's why the lists exist, so people don't need to discuss in person, offline. I am happy to make changes in spec if you want to suggest something and if something breaks it for you. > so I guess this driver hasn't so much in common w/ my original design. It actually has as I said earlier, it is still based on your patch. > Note that I made my original design decisions for good reaons > (see virtio-dev list). I tried to follow your patches from December on the Linux kernel list and the ID allocation one on virtio-comments list. I wasn't able to search for any other attempt at sending the specification, so may have missed something. I do understand that there were reasons why you (and me) chose a particular way of doing things and if there is a good reason of following that, then we can still modify the spec and fix things for everyone. We just need to discuss our reasoning on the list and get through with a specfication which works for everyone as this will become a standard interface for many in the future and it needs to be robust and efficient for everyone. > It already support async notifications > (IOW: irqs), had an easy upgrade path for future additions, etc. I haven't changed irqs path, we still have a separate virtqueue (named "interrupt" vq) which handles just the interrupts now. And so will be faster than what you initially suggested. In your original design you also received the responses for the requests on this virtqueue, wihch I have changed to get the response synchronously on the "command" virtqueue only. This is from one of your earlier replies: " I've been under the impression that queues only work in only one direction. (at least that's what my web research was telling). Could you please give an example how bi-directional transmission within the same queue could look like ? " It is actually possible and the right thing to do here as we aren't starting multiple requests together. The operation needs to be synchronous anyway and both request/response on the same channel work better there. Also that makes the interrupts reach faster, without additional delay due to responses. > Note #2: it's already implemented and running in the field (different > kernels, different hypervisors, ...) - it just lacked the going through > virtio comitte's formal specification process, which blocked mainlining. I understand the pain you would be reqiured to go through because of this, but this is how any open source community will work, like Linux. There will be changes in specification and code once you post it and any solutions already working in the field won't really matter during the discussion. That is why it is always the right thing to _upstream first_, so one can avoid these problems later on. Though I understand that the real world needs to move faster than community. But no one can really help in that. > Is there anything fundamentally wrong w/ my original design, why you > invented a completely new one ? Not much, and I haven't changed a lot as well. Lemme point out few things which have changed in specification since your earlier version (the code just followed the specification, that's it). - config structure - version information is replaced with virtio-features. - Irq support is added as a feature, as you suggested. - extra padding space (24 bytes) removed. If you see this patch we can now read the entire config structure in a single go. Why should anyone be required to copy extra 24 bytes? If we need more fields later, we can always do that with help of another feature-flag. So this is still extendable. - Virtqueues: we still have two virtqueues (command and interrupt), just responses are moved to the command virtqueue only, as that is more efficient. - Request/response: Request layout is same, added a new layout for response as the same layout is inefficient. - IRQ support: Initial specification had no interface for configuring irqs from the guest, I added that. That's all I can gather right now. I don't think that's a lot and it is mostly improvements only. But if there is a good reason on why we should do things differently, we can still discuss that. I am open to suggestions. Thanks -- Viresh