linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Dave Jiang <dave.jiang@intel.com>,
	vkoul@kernel.org, megha.dey@linux.intel.com, maz@kernel.org,
	bhelgaas@google.com, rafael@kernel.org,
	gregkh@linuxfoundation.org, hpa@zytor.com,
	alex.williamson@redhat.com, jacob.jun.pan@intel.com,
	ashok.raj@intel.com, jgg@mellanox.com, yi.l.liu@intel.com,
	baolu.lu@intel.com, kevin.tian@intel.com,
	sanjay.k.kumar@intel.com, tony.luck@intel.com,
	jing.lin@intel.com, dan.j.williams@intel.com,
	kwankhede@nvidia.com, eric.auger@redhat.com, parav@mellanox.com
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
	x86@kernel.org, linux-pci@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH RFC 02/15] drivers/base: Introduce a new platform-msi list
Date: Sat, 25 Apr 2020 23:13:44 +0200	[thread overview]
Message-ID: <87v9lntgjb.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <158751203902.36773.2662739280103265908.stgit@djiang5-desk3.ch.intel.com>

Dave Jiang <dave.jiang@intel.com> writes:

> From: Megha Dey <megha.dey@linux.intel.com>
>
> This is a preparatory patch to introduce Interrupt Message Store (IMS).
>
> The struct device has a linked list ('msi_list') of the MSI (msi/msi-x,
> platform-msi) descriptors of that device. This list holds only 1 type
> of descriptor since it is not possible for a device to support more
> than one of these descriptors concurrently.
>
> However, with the introduction of IMS, a device can support IMS as well
> as MSI-X at the same time. Instead of sharing this list between IMS (a
> type of platform-msi) and MSI-X descriptors, introduce a new linked list,
> platform_msi_list, which will hold all the platform-msi descriptors.
>
> Thus, msi_list will point to the MSI/MSIX descriptors of a device, while
> platform_msi_list will point to the platform-msi descriptors of a
> device.

Will point?

You're failing to explain that this actually converts the existing
platform code over to this new list. This also lacks an explanation why
this is not a functional change.

> Signed-off-by: Megha Dey <megha.dey@linux.intel.com>

Lacks an SOB from you.... 

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 139cdf7e7327..5a0116d1a8d0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1984,6 +1984,7 @@ void device_initialize(struct device *dev)
>  	set_dev_node(dev, -1);
>  #ifdef CONFIG_GENERIC_MSI_IRQ
>  	INIT_LIST_HEAD(&dev->msi_list);
> +	INIT_LIST_HEAD(&dev->platform_msi_list);

> --- a/drivers/base/platform-msi.c
> +++ b/drivers/base/platform-msi.c
> @@ -110,7 +110,8 @@ static void platform_msi_free_descs(struct device *dev, int base, int nvec)
>  {
>  	struct msi_desc *desc, *tmp;
>  
> -	list_for_each_entry_safe(desc, tmp, dev_to_msi_list(dev), list) {
> +	list_for_each_entry_safe(desc, tmp, dev_to_platform_msi_list(dev),
> +				 list) {
>  		if (desc->platform.msi_index >= base &&
>  		    desc->platform.msi_index < (base + nvec)) {
>  			list_del(&desc->list);
>  	datap = kzalloc(sizeof(*datap), GFP_KERNEL);
> @@ -255,6 +256,8 @@ int platform_msi_domain_alloc_irqs(struct device *dev, unsigned int nvec,
>  	struct platform_msi_priv_data *priv_data;
>  	int err;
>  
> +	dev->platform_msi_type = GEN_PLAT_MSI;

What the heck is GEN_PLAT_MSI? Can you please use

   1) A proper name space starting with PLATFORM_MSI_ or such

   2) A proper suffix which is self explaining.

instead of coming up with nonsensical garbage which even lacks any
explanation at the place where it is defined.

> diff --git a/include/linux/device.h b/include/linux/device.h
> index ac8e37cd716a..cbcecb14584e 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -567,6 +567,8 @@ struct device {
>  #endif
>  #ifdef CONFIG_GENERIC_MSI_IRQ
>  	struct list_head	msi_list;
> +	struct list_head	platform_msi_list;
> +	unsigned int		platform_msi_type;

You use an enum for the types so why are you not using an enum for the
struct member which stores it?

>  
> +/**
> + * list_entry_select - get the correct struct for this entry based on condition
> + * @condition:	the condition to choose a particular &struct list head pointer
> + * @ptr_a:      the &struct list_head pointer if @condition is not met.
> + * @ptr_b:      the &struct list_head pointer if @condition is met.
> + * @type:       the type of the struct this is embedded in.
> + * @member:     the name of the list_head within the struct.
> + */
> +#define list_entry_select(condition, ptr_a, ptr_b, type, member)\
> +	(condition) ? list_entry(ptr_a, type, member) :		\
> +		      list_entry(ptr_b, type, member)

This is related to $Subject in which way? It's not a entirely new
process rule that infrastructure changes which touch a completely
different subsystem have to be separate and explained and justified on
their own.

>  
> +enum platform_msi_type {
> +	NOT_PLAT_MSI = 0,

NOT_PLAT_MSI? Not used anywhere and of course equally self explaining as
the other one.

> +	GEN_PLAT_MSI = 1,
> +};
> +
>  /* Helpers to hide struct msi_desc implementation details */
>  #define msi_desc_to_dev(desc)		((desc)->dev)
>  #define dev_to_msi_list(dev)		(&(dev)->msi_list)
> @@ -140,6 +145,22 @@ struct msi_desc {
>  #define for_each_msi_entry_safe(desc, tmp, dev)	\
>  	list_for_each_entry_safe((desc), (tmp), dev_to_msi_list((dev)), list)
>  
> +#define dev_to_platform_msi_list(dev)	(&(dev)->platform_msi_list)
> +#define first_platform_msi_entry(dev)		\
> +	list_first_entry(dev_to_platform_msi_list((dev)), struct msi_desc, list)
> +#define for_each_platform_msi_entry(desc, dev)	\
> +	list_for_each_entry((desc), dev_to_platform_msi_list((dev)), list)
> +#define for_each_platform_msi_entry_safe(desc, tmp, dev)	\
> +	list_for_each_entry_safe((desc), (tmp), dev_to_platform_msi_list((dev)), list)

New lines to seperate macros are bad for readability, right? 

> +#define first_msi_entry_common(dev)	\
> +	list_first_entry_select((dev)->platform_msi_type, dev_to_platform_msi_list((dev)),	\
> +				dev_to_msi_list((dev)), struct msi_desc, list)
> +
> +#define for_each_msi_entry_common(desc, dev)	\
> +	list_for_each_entry_select((dev)->platform_msi_type, desc, dev_to_platform_msi_list((dev)), \
> +				   dev_to_msi_list((dev)), list)	\
> +
>  #ifdef CONFIG_IRQ_MSI_IOMMU
>  static inline const void *msi_desc_get_iommu_cookie(struct msi_desc *desc)
>  {
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index eb95f6106a1e..bc5f9e32387f 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -320,7 +320,7 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
>  	struct msi_desc *desc;
>  	int ret = 0;
>  
> -	for_each_msi_entry(desc, dev) {
> +	for_each_msi_entry_common(desc, dev) {

This is absolutely unreadable. What's common here? You hide the decision
which list to iterate behind a misnomed macro. 

And looking at the implementation:

> +#define for_each_msi_entry_common(desc, dev)	\
> +	list_for_each_entry_select((dev)->platform_msi_type, desc, dev_to_platform_msi_list((dev)), \
> +				   dev_to_msi_list((dev)), list)	\

So you implicitely make the decision based on:

   (dev)->platform_msi_type != 0

What? How is that ever supposed to work? The changelog says:

> However, with the introduction of IMS, a device can support IMS as well
> as MSI-X at the same time. Instead of sharing this list between IMS (a
> type of platform-msi) and MSI-X descriptors, introduce a new linked list,
> platform_msi_list, which will hold all the platform-msi descriptors.

So you are not serious about storing the decision in the device struct
and then calling into common code?

That's insane at best. There is absolutely ZERO explanation how this is
supposed to work and why this could even be remotely correct and safe.

Ever heard of the existance of function arguments?

Sorry, this is just voodoo programming and not going anywhere.

Thanks,

        tglx

  reply	other threads:[~2020-04-25 21:14 UTC|newest]

Thread overview: 89+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-21 23:33 [PATCH RFC 00/15] Add VFIO mediated device support and IMS support for the idxd driver Dave Jiang
2020-04-21 23:33 ` [PATCH RFC 01/15] drivers/base: Introduce platform_msi_ops Dave Jiang
2020-04-26  7:01   ` Greg KH
2020-04-27 21:38     ` Dave Jiang
2020-04-28  7:34       ` Greg KH
2020-04-21 23:33 ` [PATCH RFC 02/15] drivers/base: Introduce a new platform-msi list Dave Jiang
2020-04-25 21:13   ` Thomas Gleixner [this message]
2020-05-04  0:08     ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 03/15] drivers/base: Allocate/free platform-msi interrupts by group Dave Jiang
2020-04-25 21:23   ` Thomas Gleixner
2020-05-04  0:08     ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 04/15] drivers/base: Add support for a new IMS irq domain Dave Jiang
2020-04-23 20:11   ` Jason Gunthorpe
2020-05-01 22:30     ` Dey, Megha
2020-05-03 22:25       ` Jason Gunthorpe
2020-05-03 22:40         ` Dey, Megha
2020-05-03 22:46           ` Jason Gunthorpe
2020-05-04  0:25             ` Dey, Megha
2020-05-04 12:14               ` Jason Gunthorpe
2020-05-06 10:27                 ` Tian, Kevin
2020-04-25 21:38   ` Thomas Gleixner
2020-05-04  0:11     ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 05/15] ims-msi: Add mask/unmask routines Dave Jiang
2020-04-25 21:49   ` Thomas Gleixner
2020-05-04  0:16     ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 06/15] ims-msi: Enable IMS interrupts Dave Jiang
2020-04-25 22:13   ` Thomas Gleixner
2020-05-04  0:17     ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 07/15] Documentation: Interrupt Message store Dave Jiang
2020-04-23 20:04   ` Jason Gunthorpe
2020-05-01 22:32     ` Dey, Megha
2020-05-03 22:28       ` Jason Gunthorpe
2020-05-03 22:41         ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 08/15] vfio/mdev: Add a member for iommu domain in mdev_device Dave Jiang
2020-04-21 23:34 ` [PATCH RFC 09/15] vfio/type1: Save domain when attach domain to mdev Dave Jiang
2020-04-21 23:34 ` [PATCH RFC 10/15] dmaengine: idxd: add config support for readonly devices Dave Jiang
2020-04-21 23:34 ` [PATCH RFC 11/15] dmaengine: idxd: add IMS support in base driver Dave Jiang
2020-04-21 23:35 ` [PATCH RFC 12/15] dmaengine: idxd: add device support functions in prep for mdev Dave Jiang
2020-04-21 23:35 ` [PATCH RFC 13/15] dmaengine: idxd: add support for VFIO mediated device Dave Jiang
2020-04-21 23:35 ` [PATCH RFC 14/15] dmaengine: idxd: add error notification from host driver to " Dave Jiang
2020-04-21 23:35 ` [PATCH RFC 15/15] dmaengine: idxd: add ABI documentation for mediated device support Dave Jiang
2020-04-21 23:54 ` [PATCH RFC 00/15] Add VFIO mediated device support and IMS support for the idxd driver Jason Gunthorpe
2020-04-22  0:53   ` Tian, Kevin
2020-04-22 11:50     ` Jason Gunthorpe
2020-04-22 21:14       ` Raj, Ashok
2020-04-23 19:12         ` Jason Gunthorpe
2020-04-24  3:27           ` Tian, Kevin
2020-04-24 12:44             ` Jason Gunthorpe
2020-04-24 16:25               ` Tian, Kevin
2020-04-24 18:12                 ` Jason Gunthorpe
2020-04-26  5:18                   ` Tian, Kevin
2020-04-26 19:13                     ` Jason Gunthorpe
2020-04-27  3:43                       ` Alex Williamson
2020-04-27 11:58                         ` Jason Gunthorpe
2020-04-27 13:19                           ` Alex Williamson
2020-04-27 13:22                             ` Jason Gunthorpe
2020-04-27 14:18                               ` Alex Williamson
2020-04-27 14:25                                 ` Jason Gunthorpe
2020-04-27 15:41                                   ` Alex Williamson
2020-04-27 16:16                                     ` Jason Gunthorpe
2020-04-27 16:25                                       ` Dave Jiang
2020-04-27 21:56                                         ` Jason Gunthorpe
2020-04-29  9:42                               ` Tian, Kevin
2020-05-08 20:47                                 ` Raj, Ashok
2020-05-08 23:16                                   ` Jason Gunthorpe
2020-05-08 23:52                                     ` Dave Jiang
2020-05-09  0:09                                     ` Raj, Ashok
2020-05-09 12:21                                       ` Jason Gunthorpe
2020-05-13  2:29                                         ` Jason Wang
2020-05-13  8:30                                         ` Tian, Kevin
2020-05-13 12:40                                           ` Jason Gunthorpe
2020-04-27 12:13                       ` Tian, Kevin
2020-04-27 12:55                         ` Jason Gunthorpe
2020-04-22 21:24   ` Dan Williams
2020-04-23 19:17     ` Dan Williams
2020-04-23 19:49       ` Jason Gunthorpe
2020-05-01 22:31         ` Dey, Megha
2020-05-03 22:21           ` Jason Gunthorpe
2020-05-03 22:32             ` Dey, Megha
2020-04-23 19:18     ` Jason Gunthorpe
2020-05-01 22:31       ` Dey, Megha
2020-05-03 22:22         ` Jason Gunthorpe
2020-05-03 22:31           ` Dey, Megha
2020-05-03 22:36             ` Jason Gunthorpe
2020-05-04  0:20               ` Dey, Megha
2020-04-22 23:04   ` Dey, Megha
2020-04-23 19:44     ` Jason Gunthorpe
2020-05-01 22:32       ` Dey, Megha
2020-04-24  6:31   ` Jason Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87v9lntgjb.fsf@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=alex.williamson@redhat.com \
    --cc=ashok.raj@intel.com \
    --cc=baolu.lu@intel.com \
    --cc=bhelgaas@google.com \
    --cc=dan.j.williams@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=dmaengine@vger.kernel.org \
    --cc=eric.auger@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hpa@zytor.com \
    --cc=jacob.jun.pan@intel.com \
    --cc=jgg@mellanox.com \
    --cc=jing.lin@intel.com \
    --cc=kevin.tian@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=kwankhede@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=megha.dey@linux.intel.com \
    --cc=parav@mellanox.com \
    --cc=rafael@kernel.org \
    --cc=sanjay.k.kumar@intel.com \
    --cc=tony.luck@intel.com \
    --cc=vkoul@kernel.org \
    --cc=x86@kernel.org \
    --cc=yi.l.liu@intel.com \
    --subject='Re: [PATCH RFC 02/15] drivers/base: Introduce a new platform-msi list' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).