Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 8 Apr 2022 22:38:28 +0200
From:   Andrea Parri <parri.andrea@gmail.com>
To:     "Michael Kelley (LINUX)" <mikelley@microsoft.com>
Cc:     KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
        Wei Hu <weh@microsoft.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Rob Herring <robh@kernel.org>,
        Krzysztof Wilczynski <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/6] Drivers: hv: vmbus: Introduce
 vmbus_request_addr_match()
Message-ID: <20220408203828.GA305168@anparri>
References: <20220407043028.379534-1-parri.andrea@gmail.com>
 <20220407043028.379534-5-parri.andrea@gmail.com>
 <PH0PR21MB3025D745B0F3FA8893B32B39D7E99@PH0PR21MB3025.namprd21.prod.outlook.com>
 <20220408164717.GA206777@anparri>
 <PH0PR21MB30258AE4C3CD9674E953C765D7E99@PH0PR21MB3025.namprd21.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PH0PR21MB30258AE4C3CD9674E953C765D7E99@PH0PR21MB3025.namprd21.prod.outlook.com>
Precedence: bulk

> > > In the case where a specific match is required, and trans_id is
> > > valid but the addr's do not match, it looks like this function will
> > > return the addr that didn't match, without removing the entry.
> > 
> > Yes, that is consistent with the description on vmbus_request_addr_match():
> > 
> >   Returns the memory address stored at @trans_id, or VMBUS_RQST_ERROR if
> >   @trans_id is not contained in the requestor.
> > 
> > 
> > > Shouldn't it return VMBUS_RQST_ERROR in that case?
> > 
> > Can certainly be done, although I'm not sure to follow your concerns.  Can
> > you elaborate?
> > 
> 
> Having the function return "success" when it failed to match is unexpected
> for me.  There's only one invocation where we care about matching
> (in hv_compose_msi_msg).  In that invocation the purpose for matching is to
> not remove the wrong entry, and the return value is ignored.  So I think
> it all works correctly.

You're reading it wrongly: the point is that there's nothing wrong in *not
removing the "wrong entry" (or in failing to match).  In the mentioned use,
that means the channel callback has already processed "our" request, and
that we don't have to worry about the ID.  (Otherwise, i.e. if we do match,
the callback will eventually scream "Invalid transaction".)


> Just thinking out loud, maybe vmbus_request_addr_match() should be
> renamed to vmbus_request_addr_remove(), and not have a return value?

Mmh.  We have vmbus_request_addr() that (always) removes the ID; it seems
a _remove() would just add to the confusion.  And removing the return value
would mean duplicating most of vmbus_request_addr() in the "new" function.
So, I'm not convinced that's the right thing to do.  I'm inclined to leave
this patch as is (and, as usual, happy to be proven wrong).

Thanks,
  Andrea