Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: rgeTPppgYXTdSDu3AekE3jDHvYWRaxXYgGRdWWsCcaM9OGYuFAVDVQ2nH8H6J+TRu/gJ1TYzQZ
 0EMhq3XPfIhA==
IronPort-SDR: EK5Wgt/62jce1ymNozB9AcLEBud6htWOyYNWoERO8THhqspUe0CsaFjRRSoEWukDrsRVFVcpjq
 L+uCGTd68/zg==
Date:   Fri, 19 Feb 2021 15:51:59 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Robert Richter <rric@kernel.org>
Cc:     Dejin Zheng <zhengdejin5@gmail.com>, corbet@lwn.net,
        jarkko.nikula@linux.intel.com, mika.westerberg@linux.intel.com,
        bhelgaas@google.com, wsa@kernel.org, linux-doc@vger.kernel.org,
        linux-i2c@vger.kernel.org, linux-pci@vger.kernel.org, kw@linux.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 0/4] Introduce pcim_alloc_irq_vectors()
Message-ID: <YC/CfxHM2RMZLejc@smile.fi.intel.com>
References: <20210216160249.749799-1-zhengdejin5@gmail.com>
 <YC41HD422Mjh1IZK@rric.localdomain>
 <YC5zVHnRog3EX0rl@smile.fi.intel.com>
 <YC+euDIrR5apkAqp@rric.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YC+euDIrR5apkAqp@rric.localdomain>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Fri, Feb 19, 2021 at 12:19:20PM +0100, Robert Richter wrote:
> On 18.02.21 16:01:56, Andy Shevchenko wrote:
> > The problem this series solves is an imbalanced API.
> 
> This (added) API is bloated and incomplete. It adds functions without
> benefit, the only is to have a single pcim alloc function in addition
> to the pairing of alloc/free functions. I agree, it is hard to detect
> which parts are released if pcim_enable_device() is used.

No, this API solves the above mentioned problem (what makes so special about
pci_free_irq_vectors() that it should be present?) Why do we have pcim_iomap*()
variations and not the rest?

The PCIm API is horrible in the sense of being used properly. Yes, I know how
it works and I was trying to help with that, the problem is that people didn't
and don't get how it works and stream of patches like the ones that add
pci_free_irq_vectors() are coming.

> Additional, you need to go through pcim_release() to add other
> pcim_*() functions for everything else that is released there.
> Otherwise that new API is still incomplete.

True. And here is the part that most annoying right now.
Btw, I never saw you fought against these small clean ups here and there, that
*add* explicit calls to freeing resources. Also I haven't noticed anybody
trying to correct documentation.

This series is a step to a right direction.

> But this adds another
> bunch of useless functions.

Wrong. This is quite useful to have balanced APIs. How many patches you have
seen related to the PCIm imbalanced APIs? I could tell from my experience, I
saw plenty and each time I'm trying to explain how it works people don't easily
get.

> > Christoph IIRC was clear that if we want to use PCI IRQ allocation API the
> > caller must know what's going on. Hiding this behind the scenes is not good.
> > And this series unhides that.
> 
> IMO, this is more a documentation issue. pcim_enable_device() must be
> better documented and list all enable/alloc functions that are going
> to be released out of the box later.
> 
> Even better, make sure everything is managed and thus all of a pci_dev
> is released, no matter how it was setup (this could even already be
> the case).
> 
> In addition you could implement a static code checker.

It's open source, why should we do that and not what we are proposing here?
Propose your ideas and we will discuss the patches, right?

> > Also, you may go and clean up all pci_free_irq_vectors() when
> > pcim_enable_device() is called, but I guess you will get painful process and
> > rejection in a pile of cases.
> 
> Why should something be rejected if it is not correctly freed?

Why it's not correctly freed? The function is idempotent.

> Even if pci_free_irq_vectors() is called, pcim_release() will not
> complain if it was already freed before. So using
> pci_free_irq_vectors() is ok even in conjunction with
> pcim_enable_device().

No, it's not okay from API namespace / semantics perspective.

> In the end, let's make sure everything is released in pci_dev if it is
> managed and document this.

Feel free to submit a patch!

-- 
With Best Regards,
Andy Shevchenko