Re: [PATCH v5 2/3] misc/pvpanic: probe multiple instances

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Mar 16, 2021 at 02:20:28PM +0200, Mihai Carabas wrote:
> Create the mecahism that allows multiple pvpanic instances to call
> pvpanic_probe and receive panic events. A global list will retain all the
> mapped addresses where to write panic events.
> 
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  drivers/misc/pvpanic/pvpanic-mmio.c |  11 ++--
>  drivers/misc/pvpanic/pvpanic.c      | 105 +++++++++++++++++++++++++++++-------
>  drivers/misc/pvpanic/pvpanic.h      |   6 +--
>  3 files changed, 92 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic/pvpanic-mmio.c b/drivers/misc/pvpanic/pvpanic-mmio.c
> index 801d892..62ab8d7 100644
> --- a/drivers/misc/pvpanic/pvpanic-mmio.c
> +++ b/drivers/misc/pvpanic/pvpanic-mmio.c
> @@ -54,7 +54,7 @@ static ssize_t events_store(struct device *dev,  struct device_attribute *attr,
>  
>  	events = tmp;
>  
> -	pvpanic_set_events(events);
> +	pvpanic_set_events(base, events);
>  
>  	return count;
>  
> @@ -72,7 +72,6 @@ static int pvpanic_mmio_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct resource *res;
> -	void __iomem *base;
>  
>  	res = platform_get_mem_or_io(pdev, 0);
>  	if (!res)
> @@ -97,17 +96,13 @@ static int pvpanic_mmio_probe(struct platform_device *pdev)
>  	capability &= ioread8(base);
>  	events = capability;
>  
> -	pvpanic_probe(base, capability);
> -
> -	return 0;
> +	return pvpanic_probe(base, capability);
>  }
>  
>  static int pvpanic_mmio_remove(struct platform_device *pdev)
>  {
>  
> -	pvpanic_remove();
> -
> -	return 0;
> +	return pvpanic_remove(base);
>  }
>  
>  static const struct of_device_id pvpanic_mmio_match[] = {
> diff --git a/drivers/misc/pvpanic/pvpanic.c b/drivers/misc/pvpanic/pvpanic.c
> index 11d029c..73a82d9 100644
> --- a/drivers/misc/pvpanic/pvpanic.c
> +++ b/drivers/misc/pvpanic/pvpanic.c
> @@ -14,20 +14,35 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/types.h>
> +#include <linux/cdev.h>
> +#include <linux/list.h>
> +#include <linux/slab.h>
>  
>  #include <uapi/misc/pvpanic.h>
>  
>  #include "pvpanic.h"
>  
> -static void __iomem *base;
> -static unsigned int capability;
> -static unsigned int events;
> +struct pvpanic_instance {
> +	void __iomem *base;
> +	unsigned int capability;
> +	unsigned int events;
> +	struct list_head list;
> +};
> +
> +struct list_head pvpanic_list;
> +spinlock_t pvpanic_lock;
>  
>  static void
>  pvpanic_send_event(unsigned int event)
>  {
> -	if (event & capability & events)
> -		iowrite8(event, base);
> +	struct pvpanic_instance *pi_cur;
> +
> +	spin_lock(&pvpanic_lock);
> +	list_for_each_entry(pi_cur, &pvpanic_list, list) {
> +		if (event & pi_cur->capability & pi_cur->events)
> +			iowrite8(event, pi_cur->base);
> +	}
> +	spin_unlock(&pvpanic_lock);
>  }
>  
>  static int
> @@ -49,29 +64,81 @@
>  	.priority = 1, /* let this called before broken drm_fb_helper */
>  };
>  
> -void pvpanic_probe(void __iomem *pbase, unsigned int dev_cap)
> +int pvpanic_probe(void __iomem *pbase, unsigned int dev_cap)
>  {
> -	base = pbase;
> -	capability = dev_cap;
> -	events = capability;
> +	struct pvpanic_instance *pi;
> +
> +	if (!pbase)
> +		return -EINVAL;
> +
> +	pi = kmalloc(sizeof(*pi), GFP_ATOMIC);
> +	if (!pi)
> +		return -ENOMEM;
> +
> +	pi->base = pbase;
> +	pi->capability = dev_cap;
> +	pi->events = dev_cap;
> +	spin_lock(&pvpanic_lock);
> +	list_add(&pi->list, &pvpanic_list);
> +	spin_unlock(&pvpanic_lock);
>  
> -	if (capability)
> -		atomic_notifier_chain_register(&panic_notifier_list,
> -					       &pvpanic_panic_nb);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(pvpanic_probe);
>  
> -void pvpanic_remove(void)
> +int pvpanic_remove(void __iomem *pbase)
>  {
> -	if (capability)
> -		atomic_notifier_chain_unregister(&panic_notifier_list,
> -						 &pvpanic_panic_nb);
> -	base = NULL;
> +	struct pvpanic_instance *pi_cur, *pi_next;
> +
> +	if (!pbase)
> +		return -EINVAL;
> +
> +	spin_lock(&pvpanic_lock);
> +	list_for_each_entry_safe(pi_cur, pi_next, &pvpanic_list, list) {
> +		if (pi_cur->base == pbase) {
> +			list_del(&pi_cur->list);
> +			kfree(pi_cur);
> +			break;
> +		}
> +	}
> +	spin_unlock(&pvpanic_lock);
> +
> +	return 0;
>  }

As "remove" can not fail, this should just stay a void function.  People
are working to remove the int return value from remove calls in the
kernel right now, please do not add new ones if at all possible.

thanks,

greg k-h