[PATCH] platform: chrome: Allocate sensorhub resource before claiming sensors

[PATCH] platform: chrome: Allocate sensorhub resource before claiming sensors

[Gwendal Grignou]

Allocate callbacks array before enumerating the sensors:
The probe routine for these sensors (for instance cros_ec_sensors_probe)
can be called within the sensorhub probe routine
(cros_ec_sensors_probe())

Fixes: 145d59baff594 ("platform/chrome: cros_ec_sensorhub: Add FIFO support")

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/platform/chrome/cros_ec_sensorhub.c   | 80 +++++++++++--------
 .../platform/chrome/cros_ec_sensorhub_ring.c  | 73 ++++++++++-------
 .../linux/platform_data/cros_ec_sensorhub.h   |  1 +
 3 files changed, 93 insertions(+), 61 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c
index b7f2c00db5e1e..9c4af76a9956e 100644
--- a/drivers/platform/chrome/cros_ec_sensorhub.c
+++ b/drivers/platform/chrome/cros_ec_sensorhub.c
@@ -52,28 +52,15 @@ static int cros_ec_sensorhub_register(struct device *dev,
 	int sensor_type[MOTIONSENSE_TYPE_MAX] = { 0 };
 	struct cros_ec_command *msg = sensorhub->msg;
 	struct cros_ec_dev *ec = sensorhub->ec;
-	int ret, i, sensor_num;
+	int ret, i;
 	char *name;
 
-	sensor_num = cros_ec_get_sensor_count(ec);
-	if (sensor_num < 0) {
-		dev_err(dev,
-			"Unable to retrieve sensor information (err:%d)\n",
-			sensor_num);
-		return sensor_num;
-	}
-
-	sensorhub->sensor_num = sensor_num;
-	if (sensor_num == 0) {
-		dev_err(dev, "Zero sensors reported.\n");
-		return -EINVAL;
-	}
 
 	msg->version = 1;
 	msg->insize = sizeof(struct ec_response_motion_sense);
 	msg->outsize = sizeof(struct ec_params_motion_sense);
 
-	for (i = 0; i < sensor_num; i++) {
+	for (i = 0; i < sensorhub->sensor_num; i++) {
 		sensorhub->params->cmd = MOTIONSENSE_CMD_INFO;
 		sensorhub->params->info.sensor_num = i;
 
@@ -140,8 +127,7 @@ static int cros_ec_sensorhub_probe(struct platform_device *pdev)
 	struct cros_ec_dev *ec = dev_get_drvdata(dev->parent);
 	struct cros_ec_sensorhub *data;
 	struct cros_ec_command *msg;
-	int ret;
-	int i;
+	int ret, i, sensor_num;
 
 	msg = devm_kzalloc(dev, sizeof(struct cros_ec_command) +
 			   max((u16)sizeof(struct ec_params_motion_sense),
@@ -166,10 +152,52 @@ static int cros_ec_sensorhub_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, data);
 
 	/* Check whether this EC is a sensor hub. */
-	if (cros_ec_check_features(data->ec, EC_FEATURE_MOTION_SENSE)) {
+	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE)) {
+		sensor_num = cros_ec_get_sensor_count(ec);
+		if (sensor_num < 0) {
+			dev_err(dev,
+				"Unable to retrieve sensor information (err:%d)\n",
+				sensor_num);
+			return sensor_num;
+		}
+		if (sensor_num == 0) {
+			dev_err(dev, "Zero sensors reported.\n");
+			return -EINVAL;
+		}
+		data->sensor_num = sensor_num;
+
+		/*
+		 * Prepare the ring handler before enumering the
+		 * sensors.
+		 */
+		if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
+			ret = cros_ec_sensorhub_ring_allocate(data);
+			if (ret)
+				return ret;
+		}
+
+		/* Enumerate the sensors.*/
 		ret = cros_ec_sensorhub_register(dev, data);
 		if (ret)
 			return ret;
+
+		/*
+		 * When the EC does not have a FIFO, the sensors will query
+		 * their data themselves via sysfs or a software trigger.
+		 */
+		if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
+			ret = cros_ec_sensorhub_ring_add(data);
+			if (ret)
+				return ret;
+			/*
+			 * The msg and its data is not under the control of the
+			 * ring handler.
+			 */
+			return devm_add_action_or_reset(dev,
+					cros_ec_sensorhub_ring_remove,
+					data);
+		}
+
 	} else {
 		/*
 		 * If the device has sensors but does not claim to
@@ -184,22 +212,6 @@ static int cros_ec_sensorhub_probe(struct platform_device *pdev)
 		}
 	}
 
-	/*
-	 * If the EC does not have a FIFO, the sensors will query their data
-	 * themselves via sysfs or a software trigger.
-	 */
-	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
-		ret = cros_ec_sensorhub_ring_add(data);
-		if (ret)
-			return ret;
-		/*
-		 * The msg and its data is not under the control of the ring
-		 * handler.
-		 */
-		return devm_add_action_or_reset(dev,
-						cros_ec_sensorhub_ring_remove,
-						data);
-	}
 
 	return 0;
 }
diff --git a/drivers/platform/chrome/cros_ec_sensorhub_ring.c b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
index c48e5b38a4417..24e48d96ed766 100644
--- a/drivers/platform/chrome/cros_ec_sensorhub_ring.c
+++ b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
@@ -957,17 +957,15 @@ static int cros_ec_sensorhub_event(struct notifier_block *nb,
 }
 
 /**
- * cros_ec_sensorhub_ring_add() - Add the FIFO functionality if the EC
- *				  supports it.
+ * cros_ec_sensorhub_ring_allocate() - Prepare the FIFO functionality if the EC
+ *				       supports it.
  *
  * @sensorhub : Sensor Hub object.
  *
  * Return: 0 on success.
  */
-int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub)
+int cros_ec_sensorhub_ring_allocate(struct cros_ec_sensorhub *sensorhub)
 {
-	struct cros_ec_dev *ec = sensorhub->ec;
-	int ret;
 	int fifo_info_length =
 		sizeof(struct ec_response_motion_sense_fifo_info) +
 		sizeof(u16) * sensorhub->sensor_num;
@@ -978,6 +976,49 @@ int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub)
 	if (!sensorhub->fifo_info)
 		return -ENOMEM;
 
+	/*
+	 * Allocate the callback area based on the number of sensors.
+	 * Add one for the sensor ring.
+	 */
+	sensorhub->push_data = devm_kcalloc(sensorhub->dev,
+			sensorhub->sensor_num,
+			sizeof(*sensorhub->push_data),
+			GFP_KERNEL);
+	if (!sensorhub->push_data)
+		return -ENOMEM;
+
+	sensorhub->tight_timestamps = cros_ec_check_features(
+			sensorhub->ec,
+			EC_FEATURE_MOTION_SENSE_TIGHT_TIMESTAMPS);
+
+	if (sensorhub->tight_timestamps) {
+		sensorhub->batch_state = devm_kcalloc(sensorhub->dev,
+				sensorhub->sensor_num,
+				sizeof(*sensorhub->batch_state),
+				GFP_KERNEL);
+		if (!sensorhub->batch_state)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+/**
+ * cros_ec_sensorhub_ring_add() - Add the FIFO functionality if the EC
+ *				  supports it.
+ *
+ * @sensorhub : Sensor Hub object.
+ *
+ * Return: 0 on success.
+ */
+int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub)
+{
+	struct cros_ec_dev *ec = sensorhub->ec;
+	int ret;
+	int fifo_info_length =
+		sizeof(struct ec_response_motion_sense_fifo_info) +
+		sizeof(u16) * sensorhub->sensor_num;
+
 	/* Retrieve FIFO information */
 	sensorhub->msg->version = 2;
 	sensorhub->params->cmd = MOTIONSENSE_CMD_FIFO_INFO;
@@ -998,31 +1039,9 @@ int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub)
 	if (!sensorhub->ring)
 		return -ENOMEM;
 
-	/*
-	 * Allocate the callback area based on the number of sensors.
-	 */
-	sensorhub->push_data = devm_kcalloc(
-			sensorhub->dev, sensorhub->sensor_num,
-			sizeof(*sensorhub->push_data),
-			GFP_KERNEL);
-	if (!sensorhub->push_data)
-		return -ENOMEM;
-
 	sensorhub->fifo_timestamp[CROS_EC_SENSOR_LAST_TS] =
 		cros_ec_get_time_ns();
 
-	sensorhub->tight_timestamps = cros_ec_check_features(
-			ec, EC_FEATURE_MOTION_SENSE_TIGHT_TIMESTAMPS);
-
-	if (sensorhub->tight_timestamps) {
-		sensorhub->batch_state = devm_kcalloc(sensorhub->dev,
-				sensorhub->sensor_num,
-				sizeof(*sensorhub->batch_state),
-				GFP_KERNEL);
-		if (!sensorhub->batch_state)
-			return -ENOMEM;
-	}
-
 	/* Register the notifier that will act as a top half interrupt. */
 	sensorhub->notifier.notifier_call = cros_ec_sensorhub_event;
 	ret = blocking_notifier_chain_register(&ec->ec_dev->event_notifier,
diff --git a/include/linux/platform_data/cros_ec_sensorhub.h b/include/linux/platform_data/cros_ec_sensorhub.h
index c588be843f61b..0ecce6aa69d5e 100644
--- a/include/linux/platform_data/cros_ec_sensorhub.h
+++ b/include/linux/platform_data/cros_ec_sensorhub.h
@@ -185,6 +185,7 @@ int cros_ec_sensorhub_register_push_data(struct cros_ec_sensorhub *sensorhub,
 void cros_ec_sensorhub_unregister_push_data(struct cros_ec_sensorhub *sensorhub,
 					    u8 sensor_num);
 
+int cros_ec_sensorhub_ring_allocate(struct cros_ec_sensorhub *sensorhub);
 int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub);
 void cros_ec_sensorhub_ring_remove(void *arg);
 int cros_ec_sensorhub_ring_fifo_enable(struct cros_ec_sensorhub *sensorhub,
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH] platform: chrome: Allocate sensorhub resource before claiming sensors

[Doug Anderson]

Hi,

On Mon, Apr 27, 2020 at 3:59 PM Gwendal Grignou <gwendal@chromium.org> wrote:
>
> Allocate callbacks array before enumerating the sensors:
> The probe routine for these sensors (for instance cros_ec_sensors_probe)
> can be called within the sensorhub probe routine
> (cros_ec_sensors_probe())
>
> Fixes: 145d59baff594 ("platform/chrome: cros_ec_sensorhub: Add FIFO support")
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_sensorhub.c   | 80 +++++++++++--------
>  .../platform/chrome/cros_ec_sensorhub_ring.c  | 73 ++++++++++-------
>  .../linux/platform_data/cros_ec_sensorhub.h   |  1 +
>  3 files changed, 93 insertions(+), 61 deletions(-)

I haven't done any review of this patch, but before this patch on my
hardware enabling 'CONFIG_IIO_CROS_EC_SENSORS_CORE=y' would cause a
NULL pointer dereference at boot.  After picking this patch the NULL
pointer dereference is gone.  The details were in
<https://crbug.com/1074471>.  I haven't done any other testing than
that, though.  ;-)  If it's useful:

Reported-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

-Doug

Re: [PATCH] platform: chrome: Allocate sensorhub resource before claiming sensors

[Enric Balletbo i Serra]

Hi Gwendal,

Thank you for your patch.

On 28/4/20 0:59, Gwendal Grignou wrote:
> Allocate callbacks array before enumerating the sensors:
> The probe routine for these sensors (for instance cros_ec_sensors_probe)
> can be called within the sensorhub probe routine
> (cros_ec_sensors_probe())
> 
> Fixes: 145d59baff594 ("platform/chrome: cros_ec_sensorhub: Add FIFO support")
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>

Applied as a fix for 5.7

> ---
>  drivers/platform/chrome/cros_ec_sensorhub.c   | 80 +++++++++++--------
>  .../platform/chrome/cros_ec_sensorhub_ring.c  | 73 ++++++++++-------
>  .../linux/platform_data/cros_ec_sensorhub.h   |  1 +
>  3 files changed, 93 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c
> index b7f2c00db5e1e..9c4af76a9956e 100644
> --- a/drivers/platform/chrome/cros_ec_sensorhub.c
> +++ b/drivers/platform/chrome/cros_ec_sensorhub.c
> @@ -52,28 +52,15 @@ static int cros_ec_sensorhub_register(struct device *dev,
>  	int sensor_type[MOTIONSENSE_TYPE_MAX] = { 0 };
>  	struct cros_ec_command *msg = sensorhub->msg;
>  	struct cros_ec_dev *ec = sensorhub->ec;
> -	int ret, i, sensor_num;
> +	int ret, i;
>  	char *name;
>  
> -	sensor_num = cros_ec_get_sensor_count(ec);
> -	if (sensor_num < 0) {
> -		dev_err(dev,
> -			"Unable to retrieve sensor information (err:%d)\n",
> -			sensor_num);
> -		return sensor_num;
> -	}
> -
> -	sensorhub->sensor_num = sensor_num;
> -	if (sensor_num == 0) {
> -		dev_err(dev, "Zero sensors reported.\n");
> -		return -EINVAL;
> -	}
>  
>  	msg->version = 1;
>  	msg->insize = sizeof(struct ec_response_motion_sense);
>  	msg->outsize = sizeof(struct ec_params_motion_sense);
>  
> -	for (i = 0; i < sensor_num; i++) {
> +	for (i = 0; i < sensorhub->sensor_num; i++) {
>  		sensorhub->params->cmd = MOTIONSENSE_CMD_INFO;
>  		sensorhub->params->info.sensor_num = i;
>  
> @@ -140,8 +127,7 @@ static int cros_ec_sensorhub_probe(struct platform_device *pdev)
>  	struct cros_ec_dev *ec = dev_get_drvdata(dev->parent);
>  	struct cros_ec_sensorhub *data;
>  	struct cros_ec_command *msg;
> -	int ret;
> -	int i;
> +	int ret, i, sensor_num;
>  
>  	msg = devm_kzalloc(dev, sizeof(struct cros_ec_command) +
>  			   max((u16)sizeof(struct ec_params_motion_sense),
> @@ -166,10 +152,52 @@ static int cros_ec_sensorhub_probe(struct platform_device *pdev)
>  	dev_set_drvdata(dev, data);
>  
>  	/* Check whether this EC is a sensor hub. */
> -	if (cros_ec_check_features(data->ec, EC_FEATURE_MOTION_SENSE)) {
> +	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE)) {
> +		sensor_num = cros_ec_get_sensor_count(ec);
> +		if (sensor_num < 0) {
> +			dev_err(dev,
> +				"Unable to retrieve sensor information (err:%d)\n",
> +				sensor_num);
> +			return sensor_num;
> +		}
> +		if (sensor_num == 0) {
> +			dev_err(dev, "Zero sensors reported.\n");
> +			return -EINVAL;
> +		}
> +		data->sensor_num = sensor_num;
> +
> +		/*
> +		 * Prepare the ring handler before enumering the
> +		 * sensors.
> +		 */
> +		if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
> +			ret = cros_ec_sensorhub_ring_allocate(data);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		/* Enumerate the sensors.*/
>  		ret = cros_ec_sensorhub_register(dev, data);
>  		if (ret)
>  			return ret;
> +
> +		/*
> +		 * When the EC does not have a FIFO, the sensors will query
> +		 * their data themselves via sysfs or a software trigger.
> +		 */
> +		if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
> +			ret = cros_ec_sensorhub_ring_add(data);
> +			if (ret)
> +				return ret;
> +			/*
> +			 * The msg and its data is not under the control of the
> +			 * ring handler.
> +			 */
> +			return devm_add_action_or_reset(dev,
> +					cros_ec_sensorhub_ring_remove,
> +					data);
> +		}
> +
>  	} else {
>  		/*
>  		 * If the device has sensors but does not claim to
> @@ -184,22 +212,6 @@ static int cros_ec_sensorhub_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	/*
> -	 * If the EC does not have a FIFO, the sensors will query their data
> -	 * themselves via sysfs or a software trigger.
> -	 */
> -	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
> -		ret = cros_ec_sensorhub_ring_add(data);
> -		if (ret)
> -			return ret;
> -		/*
> -		 * The msg and its data is not under the control of the ring
> -		 * handler.
> -		 */
> -		return devm_add_action_or_reset(dev,
> -						cros_ec_sensorhub_ring_remove,
> -						data);
> -	}
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/chrome/cros_ec_sensorhub_ring.c b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
> index c48e5b38a4417..24e48d96ed766 100644
> --- a/drivers/platform/chrome/cros_ec_sensorhub_ring.c
> +++ b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
> @@ -957,17 +957,15 @@ static int cros_ec_sensorhub_event(struct notifier_block *nb,
>  }
>  
>  /**
> - * cros_ec_sensorhub_ring_add() - Add the FIFO functionality if the EC
> - *				  supports it.
> + * cros_ec_sensorhub_ring_allocate() - Prepare the FIFO functionality if the EC
> + *				       supports it.
>   *
>   * @sensorhub : Sensor Hub object.
>   *
>   * Return: 0 on success.
>   */
> -int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub)
> +int cros_ec_sensorhub_ring_allocate(struct cros_ec_sensorhub *sensorhub)
>  {
> -	struct cros_ec_dev *ec = sensorhub->ec;
> -	int ret;
>  	int fifo_info_length =
>  		sizeof(struct ec_response_motion_sense_fifo_info) +
>  		sizeof(u16) * sensorhub->sensor_num;
> @@ -978,6 +976,49 @@ int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub)
>  	if (!sensorhub->fifo_info)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Allocate the callback area based on the number of sensors.
> +	 * Add one for the sensor ring.
> +	 */
> +	sensorhub->push_data = devm_kcalloc(sensorhub->dev,
> +			sensorhub->sensor_num,
> +			sizeof(*sensorhub->push_data),
> +			GFP_KERNEL);
> +	if (!sensorhub->push_data)
> +		return -ENOMEM;
> +
> +	sensorhub->tight_timestamps = cros_ec_check_features(
> +			sensorhub->ec,
> +			EC_FEATURE_MOTION_SENSE_TIGHT_TIMESTAMPS);
> +
> +	if (sensorhub->tight_timestamps) {
> +		sensorhub->batch_state = devm_kcalloc(sensorhub->dev,
> +				sensorhub->sensor_num,
> +				sizeof(*sensorhub->batch_state),
> +				GFP_KERNEL);
> +		if (!sensorhub->batch_state)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * cros_ec_sensorhub_ring_add() - Add the FIFO functionality if the EC
> + *				  supports it.
> + *
> + * @sensorhub : Sensor Hub object.
> + *
> + * Return: 0 on success.
> + */
> +int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub)
> +{
> +	struct cros_ec_dev *ec = sensorhub->ec;
> +	int ret;
> +	int fifo_info_length =
> +		sizeof(struct ec_response_motion_sense_fifo_info) +
> +		sizeof(u16) * sensorhub->sensor_num;
> +
>  	/* Retrieve FIFO information */
>  	sensorhub->msg->version = 2;
>  	sensorhub->params->cmd = MOTIONSENSE_CMD_FIFO_INFO;
> @@ -998,31 +1039,9 @@ int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub)
>  	if (!sensorhub->ring)
>  		return -ENOMEM;
>  
> -	/*
> -	 * Allocate the callback area based on the number of sensors.
> -	 */
> -	sensorhub->push_data = devm_kcalloc(
> -			sensorhub->dev, sensorhub->sensor_num,
> -			sizeof(*sensorhub->push_data),
> -			GFP_KERNEL);
> -	if (!sensorhub->push_data)
> -		return -ENOMEM;
> -
>  	sensorhub->fifo_timestamp[CROS_EC_SENSOR_LAST_TS] =
>  		cros_ec_get_time_ns();
>  
> -	sensorhub->tight_timestamps = cros_ec_check_features(
> -			ec, EC_FEATURE_MOTION_SENSE_TIGHT_TIMESTAMPS);
> -
> -	if (sensorhub->tight_timestamps) {
> -		sensorhub->batch_state = devm_kcalloc(sensorhub->dev,
> -				sensorhub->sensor_num,
> -				sizeof(*sensorhub->batch_state),
> -				GFP_KERNEL);
> -		if (!sensorhub->batch_state)
> -			return -ENOMEM;
> -	}
> -
>  	/* Register the notifier that will act as a top half interrupt. */
>  	sensorhub->notifier.notifier_call = cros_ec_sensorhub_event;
>  	ret = blocking_notifier_chain_register(&ec->ec_dev->event_notifier,
> diff --git a/include/linux/platform_data/cros_ec_sensorhub.h b/include/linux/platform_data/cros_ec_sensorhub.h
> index c588be843f61b..0ecce6aa69d5e 100644
> --- a/include/linux/platform_data/cros_ec_sensorhub.h
> +++ b/include/linux/platform_data/cros_ec_sensorhub.h
> @@ -185,6 +185,7 @@ int cros_ec_sensorhub_register_push_data(struct cros_ec_sensorhub *sensorhub,
>  void cros_ec_sensorhub_unregister_push_data(struct cros_ec_sensorhub *sensorhub,
>  					    u8 sensor_num);
>  
> +int cros_ec_sensorhub_ring_allocate(struct cros_ec_sensorhub *sensorhub);
>  int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub);
>  void cros_ec_sensorhub_ring_remove(void *arg);
>  int cros_ec_sensorhub_ring_fifo_enable(struct cros_ec_sensorhub *sensorhub,
>