Message-ID: <54EF28C5.7020001@suse.de>
Date: Thu, 26 Feb 2015 15:08:05 +0100
From: Alexander Graf <agraf@suse.de>
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.4.0
MIME-Version: 1.0
To: "J. German Rivera" <German.Rivera@freescale.com>,
        gregkh@linuxfoundation.org, arnd@arndb.de,
        linux-kernel@vger.kernel.org
CC: stuart.yoder@freescale.com, Kim.Phillips@freescale.com,
        scottwood@freescale.com, bhamciu1@freescale.com, R89243@freescale.com,
        Geoff.Thorpe@freescale.com, bhupesh.sharma@freescale.com,
        nir.erez@freescale.com, richard.schmitt@freescale.com
Subject: Re: [PATCH v7 3/3] drivers/bus: Device driver for FSL-MC DPRC devices
References: <1424816514-26369-1-git-send-email-German.Rivera@freescale.com> <1424816514-26369-4-git-send-email-German.Rivera@freescale.com>
In-Reply-To: <1424816514-26369-4-git-send-email-German.Rivera@freescale.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 10351
Lines: 344


On 24.02.15 23:21, J. German Rivera wrote:
> A DPRC (Data Path Resource Container) is an isolation device
> that contains a set of DPAA networking devices to be
> assigned to an isolation domain (e.g., a virtual machine).
> 
> Signed-off-by: J. German Rivera <German.Rivera@freescale.com>
> Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com>
> ---
> Changes in v7:
> - Create fsl_mc_io object in dprc_probe()
> 
> Changes in v6:
> - Fixed new checkpatch warnings
> 
> Changes in v5:
> None
> 
> Changes in v4:
> - Addressed comments from Alex Graf:
>   * Fixed typo in comment
> 
> Changes in v3:
> - Addressed comments from Kim Phillips:
>   * Renamed files:
>     drivers/bus/fsl-mc/fsl_mc_dprc.c -> drivers/bus/fsl-mc/dprc-driver.c
> 
> - Addressed comments from Timur Tabi:
>   * Changed dprc_scan_container() to just return dprc_scan_objects()
>   * Changed all functions that had goto out/error when no common cleanup
>     was done, to just have multiple return points.
>   * Replaced error cleanup boolean flags with multiple exit points.
>   * REmoved __must_chewck from dprc_scan_*() functions
> 
> Changes in v2:
> - Addressed comments from Kim Phillips:
>   * Fix warning in drivers/bus/fsl-mc/fsl_mc_dprc.c:173
>   * Fixed linker errors when MC bus driver built as module
> 
>  drivers/bus/fsl-mc/Makefile      |   3 +-
>  drivers/bus/fsl-mc/dprc-driver.c | 384 +++++++++++++++++++++++++++++++++++++++
>  drivers/bus/fsl-mc/mc-bus.c      |   8 +
>  include/linux/fsl/mc-private.h   |  10 +
>  4 files changed, 404 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/bus/fsl-mc/dprc-driver.c
> 
> diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
> index decd339..424e58e 100644
> --- a/drivers/bus/fsl-mc/Makefile
> +++ b/drivers/bus/fsl-mc/Makefile
> @@ -10,5 +10,6 @@ obj-$(CONFIG_FSL_MC_BUS) += mc-bus-driver.o
>  mc-bus-driver-objs := mc-bus.o \
>  		      mc-sys.o \
>  		      dprc.o \
> -		      dpmng.o
> +		      dpmng.o \
> +		      dprc-driver.o
> 
> diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
> new file mode 100644
> index 0000000..a231d00
> --- /dev/null
> +++ b/drivers/bus/fsl-mc/dprc-driver.c
> @@ -0,0 +1,384 @@
> +/*
> + * Freescale data path resource container (DPRC) driver
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: German Rivera <German.Rivera@freescale.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/fsl/mc-private.h>
> +#include <linux/fsl/mc-sys.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include "dprc-cmd.h"
> +
> +struct dprc_child_objs {
> +	int child_count;
> +	struct dprc_obj_desc *child_array;
> +};
> +
> +static int __fsl_mc_device_remove_if_not_in_mc(struct device *dev, void *data)
> +{
> +	int i;
> +	struct dprc_child_objs *objs;
> +	struct fsl_mc_device *mc_dev;
> +
> +	WARN_ON(!dev);
> +	WARN_ON(!data);
> +	mc_dev = to_fsl_mc_device(dev);
> +	objs = data;
> +
> +	for (i = 0; i < objs->child_count; i++) {
> +		struct dprc_obj_desc *obj_desc = &objs->child_array[i];
> +
> +		if (strlen(obj_desc->type) != 0 &&
> +		    FSL_MC_DEVICE_MATCH(mc_dev, obj_desc))
> +			break;
> +	}
> +
> +	if (i == objs->child_count)
> +		fsl_mc_device_remove(mc_dev);
> +
> +	return 0;
> +}
> +
> +static int __fsl_mc_device_remove(struct device *dev, void *data)
> +{
> +	WARN_ON(!dev);
> +	WARN_ON(data);
> +	fsl_mc_device_remove(to_fsl_mc_device(dev));
> +	return 0;
> +}
> +
> +/**
> + * dprc_remove_devices - Removes devices for objects removed from a DPRC
> + *
> + * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC object
> + * @obj_desc_array: array of object descriptors for child objects currently
> + * present in the DPRC in the MC.
> + * @num_child_objects_in_mc: number of entries in obj_desc_array
> + *
> + * Synchronizes the state of the Linux bus driver with the actual state of
> + * the MC by removing devices that represent MC objects that have
> + * been dynamically removed in the physical DPRC.
> + */
> +static void dprc_remove_devices(struct fsl_mc_device *mc_bus_dev,
> +				struct dprc_obj_desc *obj_desc_array,
> +				int num_child_objects_in_mc)
> +{
> +	if (num_child_objects_in_mc != 0) {
> +		/*
> +		 * Remove child objects that are in the DPRC in Linux,
> +		 * but not in the MC:
> +		 */
> +		struct dprc_child_objs objs;
> +
> +		objs.child_count = num_child_objects_in_mc;
> +		objs.child_array = obj_desc_array;
> +		device_for_each_child(&mc_bus_dev->dev, &objs,
> +				      __fsl_mc_device_remove_if_not_in_mc);
> +	} else {
> +		/*
> +		 * There are no child objects for this DPRC in the MC.
> +		 * So, remove all the child devices from Linux:
> +		 */
> +		device_for_each_child(&mc_bus_dev->dev, NULL,
> +				      __fsl_mc_device_remove);
> +	}
> +}
> +
> +static int __fsl_mc_device_match(struct device *dev, void *data)
> +{
> +	struct dprc_obj_desc *obj_desc = data;
> +	struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
> +
> +	return FSL_MC_DEVICE_MATCH(mc_dev, obj_desc);
> +}
> +
> +static struct fsl_mc_device *fsl_mc_device_lookup(struct dprc_obj_desc
> +								*obj_desc,
> +						  struct fsl_mc_device
> +								*mc_bus_dev)
> +{
> +	struct device *dev;
> +
> +	dev = device_find_child(&mc_bus_dev->dev, obj_desc,
> +				__fsl_mc_device_match);
> +
> +	return dev ? to_fsl_mc_device(dev) : NULL;
> +}
> +
> +/**
> + * dprc_add_new_devices - Adds devices to the logical bus for a DPRC
> + *
> + * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC object
> + * @obj_desc_array: array of device descriptors for child devices currently
> + * present in the physical DPRC.
> + * @num_child_objects_in_mc: number of entries in obj_desc_array
> + *
> + * Synchronizes the state of the Linux bus driver with the actual
> + * state of the MC by adding objects that have been newly discovered
> + * in the physical DPRC.
> + */
> +static void dprc_add_new_devices(struct fsl_mc_device *mc_bus_dev,
> +				 struct dprc_obj_desc *obj_desc_array,
> +				 int num_child_objects_in_mc)
> +{
> +	int error;
> +	int i;
> +
> +	for (i = 0; i < num_child_objects_in_mc; i++) {
> +		struct fsl_mc_device *child_dev;
> +		struct fsl_mc_io *mc_io = NULL;
> +		struct dprc_obj_desc *obj_desc = &obj_desc_array[i];
> +
> +		if (strlen(obj_desc->type) == 0)
> +			continue;
> +
> +		/*
> +		 * Check if device is already known to Linux:
> +		 */
> +		child_dev = fsl_mc_device_lookup(obj_desc, mc_bus_dev);
> +		if (child_dev)
> +			continue;
> +
> +		error = fsl_mc_device_add(obj_desc, mc_io, &mc_bus_dev->dev,
> +					  &child_dev);
> +		if (error < 0) {
> +			if (mc_io)
> +				fsl_destroy_mc_io(mc_io);
> +
> +			continue;
> +		}
> +	}
> +}
> +
> +/**
> + * dprc_scan_objects - Discover objects in a DPRC
> + *
> + * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC object
> + *
> + * Detects objects added and removed from a DPRC and synchronizes the
> + * state of the Linux bus driver, MC by adding and removing
> + * devices accordingly.
> + */
> +int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev)
> +{
> +	int num_child_objects;
> +	int dprc_get_obj_failures;
> +	int error;
> +	struct dprc_obj_desc *child_obj_desc_array = NULL;
> +
> +	error = dprc_get_obj_count(mc_bus_dev->mc_io,
> +				   mc_bus_dev->mc_handle,
> +				   &num_child_objects);
> +	if (error < 0) {
> +		dev_err(&mc_bus_dev->dev, "dprc_get_obj_count() failed: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	if (num_child_objects != 0) {
> +		int i;
> +
> +		child_obj_desc_array =
> +		    devm_kmalloc_array(&mc_bus_dev->dev, num_child_objects,
> +				       sizeof(*child_obj_desc_array),
> +				       GFP_KERNEL);
> +		if (!child_obj_desc_array)
> +			return -ENOMEM;
> +
> +		/*
> +		 * Discover objects currently present in the physical DPRC:
> +		 */
> +		dprc_get_obj_failures = 0;
> +		for (i = 0; i < num_child_objects; i++) {
> +			struct dprc_obj_desc *obj_desc =
> +			    &child_obj_desc_array[i];
> +
> +			error = dprc_get_obj(mc_bus_dev->mc_io,
> +					     mc_bus_dev->mc_handle,
> +					     i, obj_desc);
> +			if (error < 0) {
> +				dev_err(&mc_bus_dev->dev,
> +					"dprc_get_obj(i=%d) failed: %d\n",
> +					i, error);
> +				/*
> +				 * Mark the obj entry as "invalid", by using the
> +				 * empty string as obj type:
> +				 */
> +				obj_desc->type[0] = '\0';
> +				obj_desc->id = error;
> +				dprc_get_obj_failures++;
> +				continue;
> +			}
> +
> +			dev_dbg(&mc_bus_dev->dev,
> +				"Discovered object: type %s, id %d\n",
> +				obj_desc->type, obj_desc->id);
> +		}
> +
> +		if (dprc_get_obj_failures != 0) {
> +			dev_err(&mc_bus_dev->dev,
> +				"%d out of %d devices could not be retrieved\n",
> +				dprc_get_obj_failures, num_child_objects);
> +		}
> +	}
> +
> +	dprc_remove_devices(mc_bus_dev, child_obj_desc_array,
> +			    num_child_objects);
> +
> +	dprc_add_new_devices(mc_bus_dev, child_obj_desc_array,
> +			     num_child_objects);
> +
> +	if (child_obj_desc_array)
> +		devm_kfree(&mc_bus_dev->dev, child_obj_desc_array);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dprc_scan_objects);

Why is this exported?

> +
> +/**
> + * dprc_scan_container - Scans a physical DPRC and synchronizes Linux bus state
> + *
> + * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC object
> + *
> + * Scans the physical DPRC and synchronizes the state of the Linux
> + * bus driver with the actual state of the MC by adding and removing
> + * devices as appropriate.
> + */
> +int dprc_scan_container(struct fsl_mc_device *mc_bus_dev)
> +{
> +	int error;
> +	struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_bus_dev);
> +
> +	/*
> +	 * Discover objects in the DPRC:
> +	 */
> +	mutex_lock(&mc_bus->scan_mutex);
> +	error = dprc_scan_objects(mc_bus_dev);
> +	mutex_unlock(&mc_bus->scan_mutex);
> +	return error;
> +}

Or rather why does the split exist at all?  Can't you just fold the
locking into the scan function?


Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/