diff mbox series

[v3] i2c: aspeed: fix invalid clock parameters for very large divisors

Message ID 20180921223446.82685-1-brendanhiggins@google.com
State Not Applicable, archived
Headers show
Series [v3] i2c: aspeed: fix invalid clock parameters for very large divisors | expand

Commit Message

Brendan Higgins Sept. 21, 2018, 10:34 p.m. UTC 
The function that computes clock parameters from divisors did not
respect the maximum size of the bitfields that the parameters were
written to. This fixes the bug.

This bug can be reproduced with (and this fix verified with) the test
at: https://kunit-review.googlesource.com/c/linux/+/1035/

Discovered-by-KUnit: https://kunit-review.googlesource.com/c/linux/+/1035/
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 - v2 updates the title of the patch, renames a local variable, and
   prints an error when the clock divider is clamped.
 - v3 adds a missing newline character for the new logging statement.
---
 drivers/i2c/busses/i2c-aspeed.c | 62 +++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 19 deletions(-)

Comments

Jae Hyun Yoo Sept. 21, 2018, 10:46 p.m. UTC | #1 
On 9/21/2018 3:34 PM, Brendan Higgins wrote:
> @@ -142,7 +142,8 @@ struct aspeed_i2c_bus {
>   	/* Synchronizes I/O mem access to base. */
>   	spinlock_t			lock;
>   	struct completion		cmd_complete;
> -	u32				(*get_clk_reg_val)(u32 divisor);
> +	u32				(*get_clk_reg_val)(struct device *dev,
> +							   u32 divisor);

I realized that you changed *get_clk_reg_val type in v2. You probably
need to fix below code in aspeed_i2c_probe_bus() as well to remove
a sparse warning.

bus->get_clk_reg_val = (u32 (*)(u32))match->data;

-Jae
kernel test robot Sept. 22, 2018, 6:58 a.m. UTC | #2 
Hi Brendan,

I love your patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.19-rc4 next-20180921]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Brendan-Higgins/i2c-aspeed-fix-invalid-clock-parameters-for-very-large-divisors/20180922-134643
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-aspeed.c: In function 'aspeed_i2c_probe_bus':
>> drivers/i2c/busses/i2c-aspeed.c:920:24: error: assignment from incompatible pointer type [-Werror=incompatible-pointer-types]
      bus->get_clk_reg_val = (u32 (*)(u32))match->data;
                           ^
   cc1: some warnings being treated as errors

vim +920 drivers/i2c/busses/i2c-aspeed.c

87b59ff8d Brendan Higgins 2017-07-28  875  
f327c686d Brendan Higgins 2017-06-20  876  static int aspeed_i2c_probe_bus(struct platform_device *pdev)
f327c686d Brendan Higgins 2017-06-20  877  {
87b59ff8d Brendan Higgins 2017-07-28  878  	const struct of_device_id *match;
f327c686d Brendan Higgins 2017-06-20  879  	struct aspeed_i2c_bus *bus;
f327c686d Brendan Higgins 2017-06-20  880  	struct clk *parent_clk;
f327c686d Brendan Higgins 2017-06-20  881  	struct resource *res;
f327c686d Brendan Higgins 2017-06-20  882  	int irq, ret;
f327c686d Brendan Higgins 2017-06-20  883  
f327c686d Brendan Higgins 2017-06-20  884  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
f327c686d Brendan Higgins 2017-06-20  885  	if (!bus)
f327c686d Brendan Higgins 2017-06-20  886  		return -ENOMEM;
f327c686d Brendan Higgins 2017-06-20  887  
f327c686d Brendan Higgins 2017-06-20  888  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
f327c686d Brendan Higgins 2017-06-20  889  	bus->base = devm_ioremap_resource(&pdev->dev, res);
f327c686d Brendan Higgins 2017-06-20  890  	if (IS_ERR(bus->base))
f327c686d Brendan Higgins 2017-06-20  891  		return PTR_ERR(bus->base);
f327c686d Brendan Higgins 2017-06-20  892  
f327c686d Brendan Higgins 2017-06-20  893  	parent_clk = devm_clk_get(&pdev->dev, NULL);
f327c686d Brendan Higgins 2017-06-20  894  	if (IS_ERR(parent_clk))
f327c686d Brendan Higgins 2017-06-20  895  		return PTR_ERR(parent_clk);
f327c686d Brendan Higgins 2017-06-20  896  	bus->parent_clk_frequency = clk_get_rate(parent_clk);
f327c686d Brendan Higgins 2017-06-20  897  	/* We just need the clock rate, we don't actually use the clk object. */
f327c686d Brendan Higgins 2017-06-20  898  	devm_clk_put(&pdev->dev, parent_clk);
f327c686d Brendan Higgins 2017-06-20  899  
edd20e95b Joel Stanley    2017-11-01  900  	bus->rst = devm_reset_control_get_shared(&pdev->dev, NULL);
edd20e95b Joel Stanley    2017-11-01  901  	if (IS_ERR(bus->rst)) {
edd20e95b Joel Stanley    2017-11-01  902  		dev_err(&pdev->dev,
6bc33c519 Jae Hyun Yoo    2018-07-02  903  			"missing or invalid reset controller device tree entry\n");
edd20e95b Joel Stanley    2017-11-01  904  		return PTR_ERR(bus->rst);
edd20e95b Joel Stanley    2017-11-01  905  	}
edd20e95b Joel Stanley    2017-11-01  906  	reset_control_deassert(bus->rst);
edd20e95b Joel Stanley    2017-11-01  907  
f327c686d Brendan Higgins 2017-06-20  908  	ret = of_property_read_u32(pdev->dev.of_node,
f327c686d Brendan Higgins 2017-06-20  909  				   "bus-frequency", &bus->bus_frequency);
f327c686d Brendan Higgins 2017-06-20  910  	if (ret < 0) {
f327c686d Brendan Higgins 2017-06-20  911  		dev_err(&pdev->dev,
f327c686d Brendan Higgins 2017-06-20  912  			"Could not read bus-frequency property\n");
f327c686d Brendan Higgins 2017-06-20  913  		bus->bus_frequency = 100000;
f327c686d Brendan Higgins 2017-06-20  914  	}
f327c686d Brendan Higgins 2017-06-20  915  
87b59ff8d Brendan Higgins 2017-07-28  916  	match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node);
87b59ff8d Brendan Higgins 2017-07-28  917  	if (!match)
87b59ff8d Brendan Higgins 2017-07-28  918  		bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
87b59ff8d Brendan Higgins 2017-07-28  919  	else
5799c4b2f Jae Hyun Yoo    2018-07-24 @920  		bus->get_clk_reg_val = (u32 (*)(u32))match->data;
87b59ff8d Brendan Higgins 2017-07-28  921  
f327c686d Brendan Higgins 2017-06-20  922  	/* Initialize the I2C adapter */
f327c686d Brendan Higgins 2017-06-20  923  	spin_lock_init(&bus->lock);
f327c686d Brendan Higgins 2017-06-20  924  	init_completion(&bus->cmd_complete);
f327c686d Brendan Higgins 2017-06-20  925  	bus->adap.owner = THIS_MODULE;
f327c686d Brendan Higgins 2017-06-20  926  	bus->adap.retries = 0;
f327c686d Brendan Higgins 2017-06-20  927  	bus->adap.timeout = 5 * HZ;
f327c686d Brendan Higgins 2017-06-20  928  	bus->adap.algo = &aspeed_i2c_algo;
f327c686d Brendan Higgins 2017-06-20  929  	bus->adap.dev.parent = &pdev->dev;
f327c686d Brendan Higgins 2017-06-20  930  	bus->adap.dev.of_node = pdev->dev.of_node;
f327c686d Brendan Higgins 2017-06-20  931  	strlcpy(bus->adap.name, pdev->name, sizeof(bus->adap.name));
f327c686d Brendan Higgins 2017-06-20  932  	i2c_set_adapdata(&bus->adap, bus);
f327c686d Brendan Higgins 2017-06-20  933  
f327c686d Brendan Higgins 2017-06-20  934  	bus->dev = &pdev->dev;
f327c686d Brendan Higgins 2017-06-20  935  
f327c686d Brendan Higgins 2017-06-20  936  	/* Clean up any left over interrupt state. */
f327c686d Brendan Higgins 2017-06-20  937  	writel(0, bus->base + ASPEED_I2C_INTR_CTRL_REG);
f327c686d Brendan Higgins 2017-06-20  938  	writel(0xffffffff, bus->base + ASPEED_I2C_INTR_STS_REG);
f327c686d Brendan Higgins 2017-06-20  939  	/*
f327c686d Brendan Higgins 2017-06-20  940  	 * bus.lock does not need to be held because the interrupt handler has
f327c686d Brendan Higgins 2017-06-20  941  	 * not been enabled yet.
f327c686d Brendan Higgins 2017-06-20  942  	 */
f327c686d Brendan Higgins 2017-06-20  943  	ret = aspeed_i2c_init(bus, pdev);
f327c686d Brendan Higgins 2017-06-20  944  	if (ret < 0)
f327c686d Brendan Higgins 2017-06-20  945  		return ret;
f327c686d Brendan Higgins 2017-06-20  946  
f327c686d Brendan Higgins 2017-06-20  947  	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
f327c686d Brendan Higgins 2017-06-20  948  	ret = devm_request_irq(&pdev->dev, irq, aspeed_i2c_bus_irq,
f327c686d Brendan Higgins 2017-06-20  949  			       0, dev_name(&pdev->dev), bus);
f327c686d Brendan Higgins 2017-06-20  950  	if (ret < 0)
f327c686d Brendan Higgins 2017-06-20  951  		return ret;
f327c686d Brendan Higgins 2017-06-20  952  
f327c686d Brendan Higgins 2017-06-20  953  	ret = i2c_add_adapter(&bus->adap);
f327c686d Brendan Higgins 2017-06-20  954  	if (ret < 0)
f327c686d Brendan Higgins 2017-06-20  955  		return ret;
f327c686d Brendan Higgins 2017-06-20  956  
f327c686d Brendan Higgins 2017-06-20  957  	platform_set_drvdata(pdev, bus);
f327c686d Brendan Higgins 2017-06-20  958  
f327c686d Brendan Higgins 2017-06-20  959  	dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
f327c686d Brendan Higgins 2017-06-20  960  		 bus->adap.nr, irq);
f327c686d Brendan Higgins 2017-06-20  961  
f327c686d Brendan Higgins 2017-06-20  962  	return 0;
f327c686d Brendan Higgins 2017-06-20  963  }
f327c686d Brendan Higgins 2017-06-20  964  

:::::: The code at line 920 was first introduced by commit
:::::: 5799c4b2f1dbc0166d9b1d94443deaafc6e7a070 i2c: aspeed: Add an explicit type casting for *get_clk_reg_val

:::::: TO: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
:::::: CC: Wolfram Sang <wsa@the-dreams.de>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index c258c4d9a4c0..a3bfec2c0694 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -142,7 +142,8 @@  struct aspeed_i2c_bus {
 	/* Synchronizes I/O mem access to base. */
 	spinlock_t			lock;
 	struct completion		cmd_complete;
-	u32				(*get_clk_reg_val)(u32 divisor);
+	u32				(*get_clk_reg_val)(struct device *dev,
+							   u32 divisor);
 	unsigned long			parent_clk_frequency;
 	u32				bus_frequency;
 	/* Transaction state. */
@@ -705,16 +706,27 @@  static const struct i2c_algorithm aspeed_i2c_algo = {
 #endif /* CONFIG_I2C_SLAVE */
 };
 
-static u32 aspeed_i2c_get_clk_reg_val(u32 clk_high_low_max, u32 divisor)
+static u32 aspeed_i2c_get_clk_reg_val(struct device *dev,
+				      u32 clk_high_low_mask,
+				      u32 divisor)
 {
-	u32 base_clk, clk_high, clk_low, tmp;
+	u32 base_clk_divisor, clk_high_low_max, clk_high, clk_low, tmp;
+
+	/*
+	 * SCL_high and SCL_low represent a value 1 greater than what is stored
+	 * since a zero divider is meaningless. Thus, the max value each can
+	 * store is every bit set + 1. Since SCL_high and SCL_low are added
+	 * together (see below), the max value of both is the max value of one
+	 * them times two.
+	 */
+	clk_high_low_max = (clk_high_low_mask + 1) * 2;
 
 	/*
 	 * The actual clock frequency of SCL is:
 	 *	SCL_freq = APB_freq / (base_freq * (SCL_high + SCL_low))
 	 *		 = APB_freq / divisor
 	 * where base_freq is a programmable clock divider; its value is
-	 *	base_freq = 1 << base_clk
+	 *	base_freq = 1 << base_clk_divisor
 	 * SCL_high is the number of base_freq clock cycles that SCL stays high
 	 * and SCL_low is the number of base_freq clock cycles that SCL stays
 	 * low for a period of SCL.
@@ -724,47 +736,59 @@  static u32 aspeed_i2c_get_clk_reg_val(u32 clk_high_low_max, u32 divisor)
 	 *	SCL_low	 = clk_low + 1
 	 * Thus,
 	 *	SCL_freq = APB_freq /
-	 *		((1 << base_clk) * (clk_high + 1 + clk_low + 1))
+	 *		((1 << base_clk_divisor) * (clk_high + 1 + clk_low + 1))
 	 * The documentation recommends clk_high >= clk_high_max / 2 and
 	 * clk_low >= clk_low_max / 2 - 1 when possible; this last constraint
 	 * gives us the following solution:
 	 */
-	base_clk = divisor > clk_high_low_max ?
+	base_clk_divisor = divisor > clk_high_low_max ?
 			ilog2((divisor - 1) / clk_high_low_max) + 1 : 0;
-	tmp = (divisor + (1 << base_clk) - 1) >> base_clk;
-	clk_low = tmp / 2;
-	clk_high = tmp - clk_low;
 
-	if (clk_high)
-		clk_high--;
+	if (base_clk_divisor > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK) {
+		base_clk_divisor = ASPEED_I2CD_TIME_BASE_DIVISOR_MASK;
+		clk_low = clk_high_low_mask;
+		clk_high = clk_high_low_mask;
+		dev_err(dev,
+			"clamping clock divider: divider requested, %u, is greater than largest possible divider, %u.\n",
+			divisor, (1 << base_clk_divisor) * clk_high_low_max);
+	} else {
+		tmp = (divisor + (1 << base_clk_divisor) - 1)
+				>> base_clk_divisor;
+		clk_low = tmp / 2;
+		clk_high = tmp - clk_low;
+
+		if (clk_high)
+			clk_high--;
 
-	if (clk_low)
-		clk_low--;
+		if (clk_low)
+			clk_low--;
+	}
 
 
 	return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
 		& ASPEED_I2CD_TIME_SCL_HIGH_MASK)
 			| ((clk_low << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
 			   & ASPEED_I2CD_TIME_SCL_LOW_MASK)
-			| (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
+			| (base_clk_divisor
+			   & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
 }
 
-static u32 aspeed_i2c_24xx_get_clk_reg_val(u32 divisor)
+static u32 aspeed_i2c_24xx_get_clk_reg_val(struct device *dev, u32 divisor)
 {
 	/*
 	 * clk_high and clk_low are each 3 bits wide, so each can hold a max
 	 * value of 8 giving a clk_high_low_max of 16.
 	 */
-	return aspeed_i2c_get_clk_reg_val(16, divisor);
+	return aspeed_i2c_get_clk_reg_val(dev, GENMASK(2, 0), divisor);
 }
 
-static u32 aspeed_i2c_25xx_get_clk_reg_val(u32 divisor)
+static u32 aspeed_i2c_25xx_get_clk_reg_val(struct device *dev, u32 divisor)
 {
 	/*
 	 * clk_high and clk_low are each 4 bits wide, so each can hold a max
 	 * value of 16 giving a clk_high_low_max of 32.
 	 */
-	return aspeed_i2c_get_clk_reg_val(32, divisor);
+	return aspeed_i2c_get_clk_reg_val(dev, GENMASK(3, 0), divisor);
 }
 
 /* precondition: bus.lock has been acquired. */
@@ -777,7 +801,7 @@  static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
 	clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
 			ASPEED_I2CD_TIME_THDSTA_MASK |
 			ASPEED_I2CD_TIME_TACST_MASK);
-	clk_reg_val |= bus->get_clk_reg_val(divisor);
+	clk_reg_val |= bus->get_clk_reg_val(bus->dev, divisor);
 	writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1);
 	writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2);