Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751826AbdFHOoX (ORCPT ); Thu, 8 Jun 2017 10:44:23 -0400 Received: from mail-qt0-f176.google.com ([209.85.216.176]:33303 "EHLO mail-qt0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751479AbdFHOoV (ORCPT ); Thu, 8 Jun 2017 10:44:21 -0400 MIME-Version: 1.0 In-Reply-To: <20170608161519.67e3908d@crub> References: <1494777082-13375-1-git-send-email-agust@denx.de> <20170608010928.3effdf38@crub> <20170608161519.67e3908d@crub> From: Andy Shevchenko Date: Thu, 8 Jun 2017 17:44:19 +0300 Message-ID: Subject: Re: [PATCH v5] fpga manager: Add Altera CvP driver To: Anatolij Gustschin Cc: linux-fpga@vger.kernel.org, Alan Tull , Moritz Fischer , "linux-kernel@vger.kernel.org" , matthew.gerlach@linux.intel.com, yi1.li@linux.intel.com Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v58EiSRv026848 Content-Length: 3451 Lines: 108 On Thu, Jun 8, 2017 at 5:15 PM, Anatolij Gustschin wrote: > On Thu, 8 Jun 2017 02:38:55 +0300 > Andy Shevchenko andy.shevchenko@gmail.com wrote: >>On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin wrote: >>> On Fri, 2 Jun 2017 20:43:21 +0300 >>> Andy Shevchenko andy.shevchenko@gmail.com wrote: >>Besides below comments, please, do >> >>s/VSEC_/VSE_/g >> >>for entire file. >> >>We are following PCI and Thunderbolt pattern for use of Vendor >>Specific Extended Capability. > > I can do it, but I'm just not getting why. The registers are named as VSEC > registers in the documentation, why should the code name them differently? Does your documentation decode VSEC abbreviation? What C stands for? Capability? In PCI and Thunderbolt we agreed to use word capability separately, so, either XXX_... or XXX_CAP_... to use. >>>>> + if (!timeout_us) >>>>> + return -ETIMEDOUT; >>>> >>>>Hmm... >>>>What as a user I would expect here is at least one attempt (0 -- no >>>>timeout, but try once). >>> >>> yes, this first attempt is above, please see original patch for full >>> context. >> >>Ah, it means you don't correctly use do {} while approach. >> >>Remove everything above do { and move usleep after check for the >>status inside the loop. > > Unfortunately, suggested approach has an unwanted side effect: How come? See below. > > do { > check and return if done; > > usleep_range(10, 11); > tout -= 10; > > } while (tout > 0); > > For simplicity, let's say we were asked to wait with 20 µs timeout. > Assume, that the device reports ready status after 17 µs. The first > check is done, we don't return and sleep approx. 10 µs. Then, the 2nd > check is done and we continue to wait another 10 µs and the loop ends > signalling a timeout. But in the meantime the device reported ready > status. Additional check would be needed after the loop. > In some cases the device reports ready status immediately. That's > the reason why I check first and then loop with more wait&check cycles. Just look to the rest of the code in kernel Most of the timeout related loops we have the following pattern: unsigned int retries = XXX; do { ...check for something... if (yes) return YY; ...sleep for a while... } while (--retries); if (!retries) return -ETIMEDOUT; What I'm suggesting is to follow the pattern (adjust it for your exact conditions and so on). >>>>> +static struct pci_device_id altera_cvp_id_tbl[] = { >>>>> + { PCI_VDEVICE(ALTERA, PCI_ANY_ID) }, >>>> >>>>Does it have dedicated PCI class? >>>> >>>>PCI_ANY_ID usually is too broad. >>> >>> no, it doesn't have dedicated class. >> >>Hmm... It means any device of this vendor will jump into this >>driver... Not good. > > in an early patch version I was asked by Intel people to use PCI_ANY_ID > because these devices are not set in stone. The implemented FPGA PCIe > devices can have varying IDs. probe() checks for expected capability ID > and stops if we hit a not supported device. Yeah, the problem is that every device with a such Vendor ID would be considered by this driver and PCI class would be helpful here just to reduce an impact. Capability approach works, though it's slightly more error prone. I have no other comment on this. For now it seems the only choice since such IPs are on the market, right? -- With Best Regards, Andy Shevchenko