[PATCH v2] staging: vt6655: check for memory allocation failures
There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Add return when allocation is failed.
Signed-off-by: Ji-Hun Kim
---
Change: since v1:
- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
allocated memory in the for loop after kzalloc() failed.
- In the freeing function, if td_info and rd_info are not allocated, no
needs to free.
drivers/staging/vt6655/device_main.c | 64 +---
1 file changed, 44 insertions(+), 20 deletions(-)
diff --git a/drivers/staging/vt6655/device_main.c
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..ecbba43 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = &priv->aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+ if (!desc->rd_info)
+ goto error;
if (!device_alloc_rx_buf(priv, desc))
dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
@@ -550,6 +551,10 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = &priv->aRD0Ring[0];
+
+ return;
+error:
+ device_free_rd0_ring(priv);
}
static void device_init_rd1_ring(struct vnt_private *priv)
@@ -563,7 +568,8 @@ static void device_init_rd1_ring(struct vnt_private *priv)
i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = &priv->aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+ if (!desc->rd_info)
+ goto error;
if (!device_alloc_rx_buf(priv, desc))
dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
@@ -574,6 +580,10 @@ static void device_init_rd1_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
priv->pCurrRD[1] = &priv->aRD1Ring[0];
+
+ return;
+error:
+ device_free_rd1_ring(priv);
}
static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +594,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = &priv->aRD0Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
- dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
- dev_kfree_skb(rd_info->skb);
-
- kfree(desc->rd_info);
+ if (rd_info) {
+ dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
+ priv->rx_buf_sz, DMA_FROM_DEVICE);
+ dev_kfree_skb(rd_info->skb);
+ kfree(desc->rd_info);
+ }
}
}
@@ -601,12 +611,12 @@ static void device_free_rd1_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = &priv->aRD1Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
- dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
- dev_kfree_skb(rd_info->skb);
-
- kfree(desc->rd_info);
+ if (rd_info) {
+ dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
+ priv->rx_buf_sz, DMA_FROM_DEVICE);
+ dev_kfree_skb(rd_info->skb);
+ kfree(desc->rd_info);
+ }
}
}
@@ -621,7 +631,8 @@ static void device_init_td0_ring(struct vnt_private *priv)
i++, curr += sizeof(struct vnt_tx_desc)) {
desc = &priv->apTD0Rings[i];
desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+ if (!desc->td_info)
+ goto error;
desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
@@ -632,6 +643,10 @@ static void device_init_td0_ring(struct vnt_private *priv)
if (i > 0)
priv->apTD0Rings[i-1].next_desc =
cpu_to_le32(priv->td0_pool_dma);
priv->apTailTD[0] = priv->apCurrTD[0] = &priv->apTD0Rings[0];
+
+ return;
+error:
+ device_free_td0_ring(priv);
}
static void device_init_td1_ring(struct vnt_private *priv)
@@ -646,7 +661,8 @@ static void device_init_td1_ring(struct vnt_private *priv)
i++, curr += sizeof(struct vnt_tx_desc)) {
Re: [PATCH 1/1] Drivers: hv: vmbus: Fix ring buffer signaling
On Wed, Mar 28, 2018 at 12:01:42PM -0700, Stephen Hemminger wrote:
> On Sun, 4 Mar 2018 22:24:08 -0700
> k...@exchange.microsoft.com wrote:
>
> > From: Michael Kelley
> >
> > Fix bugs in signaling the Hyper-V host when freeing space in the
> > host->guest ring buffer:
> >
> > 1. The interrupt_mask must not be used to determine whether to signal
> >on the host->guest ring buffer
> > 2. The ring buffer write_index must be read (via hv_get_bytes_to_write)
> >*after* pending_send_sz is read in order to avoid a race condition
> > 3. Comparisons with pending_send_sz must treat the "equals" case as
> >not-enough-space
> > 4. Don't signal if the pending_send_sz feature is not present. Older
> >versions of Hyper-V that don't implement this feature will poll.
> >
> > Fixes: 03bad714a161 ("vmbus: more host signalling avoidance")
> >
> > Cc: Stable # 4.14 and above
> > Signed-off-by: Michael Kelley
> > Signed-off-by: K. Y. Srinivasan
>
> What ever happened to this patch? It doesn't seem to be in char-misc,
> upstream,
> or stable kernel tree yet.
>
It was in the 4.14.31 and 4.15.14 stable releases.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: vt6655: check for memory allocation failures
On 2018/3/29 15:22, Ji-Hun Kim wrote:
There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Add return when allocation is failed.
Signed-off-by: Ji-Hun Kim
---
Change: since v1:
- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
allocated memory in the for loop after kzalloc() failed.
- In the freeing function, if td_info and rd_info are not allocated, no
needs to free.
drivers/staging/vt6655/device_main.c | 64 +---
1 file changed, 44 insertions(+), 20 deletions(-)
diff --git a/drivers/staging/vt6655/device_main.c
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..ecbba43 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = &priv->aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+ if (!desc->rd_info)
+ goto error;
if (!device_alloc_rx_buf(priv, desc))
dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
@@ -550,6 +551,10 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = &priv->aRD0Ring[0];
+
+ return;
+error:
+ device_free_rd0_ring(priv);
}
I think you should return an error number here, because
device_init_rd0_ring() is called by vnt_start().
You should also implement error handling code in vnt_start(), and let
vnt_start() returns an error number too.
The same for device_init_rd1_ring(), device_init_td0_ring() and
device_init_td1_ring().
Best wishes,
Jia-Ju Bai
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 0/6] adis16209 driver cleanup
This patchset has been introduced for the cleanup of adis16209 driver. This patchset cleans up miscellaneous code fragments and improves the code readability. After the cleanup, driver is moved from staging to mailine IIO subsystem directory. Changes in v6 -This patchset has some new patches introduced for more cleanup and from the previous version, patch *[1/6] has been included in this patch series. Shreeya Patel (6): Staging: iio: adis16209: Indent the field definitions Staging: iio: adis16209: Prefer reverse christmas tree ordering Staging: iio: adis16209: Add blank lines Staging: iio: adis16209: Remove unused headers Staging: iio: adis16209: Use GENMASK Staging: iio: adis16209: Move adis16209 driver out of staging drivers/iio/accel/Kconfig | 12 ++ drivers/iio/accel/Makefile| 1 + drivers/iio/accel/adis16209.c | 330 + drivers/staging/iio/accel/Kconfig | 12 -- drivers/staging/iio/accel/Makefile| 1 - drivers/staging/iio/accel/adis16209.c | 334 -- 6 files changed, 343 insertions(+), 347 deletions(-) create mode 100644 drivers/iio/accel/adis16209.c delete mode 100644 drivers/staging/iio/accel/adis16209.c -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 1/6] Staging: iio: adis16209: Indent the field definitions
Have indentation in field definitions to make them clearly different from the register addresses. Signed-off-by: Shreeya Patel --- Changes in v5 -Change some macro names and have indentation in the field definitions. Changes in v6 -Have indentation in the field definitions and do not change the names of the macros as the patch for changing the names has already been applied. drivers/staging/iio/accel/adis16209.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c index a8453bf..0e6366a 100644 --- a/drivers/staging/iio/accel/adis16209.c +++ b/drivers/staging/iio/accel/adis16209.c @@ -71,13 +71,13 @@ #define ADIS16209_STAT_REG 0x3C #define ADIS16209_STAT_ALARM2 BIT(9) #define ADIS16209_STAT_ALARM1 BIT(8) -#define ADIS16209_STAT_SELFTEST_FAIL_BIT 5 -#define ADIS16209_STAT_SPI_FAIL_BIT3 -#define ADIS16209_STAT_FLASH_UPT_FAIL_BIT 2 +#define ADIS16209_STAT_SELFTEST_FAIL_BIT 5 +#define ADIS16209_STAT_SPI_FAIL_BIT 3 +#define ADIS16209_STAT_FLASH_UPT_FAIL_BIT 2 /* Power supply above 3.625 V */ -#define ADIS16209_STAT_POWER_HIGH_BIT 1 +#define ADIS16209_STAT_POWER_HIGH_BIT 1 /* Power supply below 3.15 V */ -#define ADIS16209_STAT_POWER_LOW_BIT 0 +#define ADIS16209_STAT_POWER_LOW_BIT 0 #define ADIS16209_CMD_REG 0x3E #define ADIS16209_CMD_SW_RESETBIT(7) -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 2/6] Staging: iio: adis16209: Prefer reverse christmas tree ordering
Prefer reverse christmas tree ordering of declarations to
improve readability.
Signed-off-by: Shreeya Patel
---
Changes in v6
-Introduce this new patch in the series.
drivers/staging/iio/accel/adis16209.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/iio/accel/adis16209.c
b/drivers/staging/iio/accel/adis16209.c
index 0e6366a..8f4fa6b 100644
--- a/drivers/staging/iio/accel/adis16209.c
+++ b/drivers/staging/iio/accel/adis16209.c
@@ -270,9 +270,9 @@ static const struct adis_data adis16209_data = {
static int adis16209_probe(struct spi_device *spi)
{
- int ret;
- struct adis *st;
struct iio_dev *indio_dev;
+ struct adis *st;
+ int ret;
indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
if (!indio_dev)
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 3/6] Staging: iio: adis16209: Add a blank line after return statements
Add a blank line after return statements to improve the code readability. Signed-off-by: Shreeya Patel --- Changes in v6 -Introduce this new patch in the series. drivers/staging/iio/accel/adis16209.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c index 8f4fa6b..ee7e87b 100644 --- a/drivers/staging/iio/accel/adis16209.c +++ b/drivers/staging/iio/accel/adis16209.c @@ -277,6 +277,7 @@ static int adis16209_probe(struct spi_device *spi) indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); if (!indio_dev) return -ENOMEM; + st = iio_priv(indio_dev); spi_set_drvdata(spi, indio_dev); @@ -290,6 +291,7 @@ static int adis16209_probe(struct spi_device *spi) ret = adis_init(st, indio_dev, spi, &adis16209_data); if (ret) return ret; + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL); if (ret) return ret; -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] ANDROID: binder: re-order some conditions
It doesn't make any difference to runtime but I've switched these two checks to make my static checker happy. The problem is that "buffer->data_size" is user controlled and if it's less than "sizeo(*hdr)" then that means "offset" can be more than "buffer->data_size". It's just cleaner to check it in the other order. Signed-off-by: Dan Carpenter diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 764b63a5aade..00322b146469 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2058,8 +2058,8 @@ static size_t binder_validate_object(struct binder_buffer *buffer, u64 offset) struct binder_object_header *hdr; size_t object_size = 0; - if (offset > buffer->data_size - sizeof(*hdr) || - buffer->data_size < sizeof(*hdr) || + if (buffer->data_size < sizeof(*hdr) || + offset > buffer->data_size - sizeof(*hdr) || !IS_ALIGNED(offset, sizeof(u32))) return 0; ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 4/6] Staging: iio: adis16209: Remove unused headers
Remove few unused header files since the adis core handles the sysfs and buffer support. Signed-off-by: Shreeya Patel --- Changes in v6 -Introduce this new patch in the series. drivers/staging/iio/accel/adis16209.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c index ee7e87b..ed6d7c7 100644 --- a/drivers/staging/iio/accel/adis16209.c +++ b/drivers/staging/iio/accel/adis16209.c @@ -6,7 +6,6 @@ * Licensed under the GPL-2 or later. */ -#include #include #include #include @@ -16,8 +15,6 @@ #include #include -#include -#include #include #define ADIS16209_STARTUP_DELAY_MS 220 -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 5/6] Staging: iio: adis16209: Use GENMASK
Use GENMASK to improve readability and remove the local
variables used to store intermediate data.
Signed-off-by: Shreeya Patel
---
Changes in v6
-Introduce this new patch in the series.
drivers/staging/iio/accel/adis16209.c | 19 ---
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/iio/accel/adis16209.c
b/drivers/staging/iio/accel/adis16209.c
index ed6d7c7..cc50667 100644
--- a/drivers/staging/iio/accel/adis16209.c
+++ b/drivers/staging/iio/accel/adis16209.c
@@ -112,25 +112,22 @@ static int adis16209_write_raw(struct iio_dev *indio_dev,
long mask)
{
struct adis *st = iio_priv(indio_dev);
- int bits;
- s16 val16;
- u8 addr;
+ int m;
+
+ if (mask != IIO_CHAN_INFO_CALIBBIAS)
+ return -EINVAL;
- switch (mask) {
- case IIO_CHAN_INFO_CALIBBIAS:
switch (chan->type) {
case IIO_ACCEL:
case IIO_INCLI:
- bits = 14;
+ m = GENMASK(13, 0);
break;
default:
return -EINVAL;
}
- val16 = val & ((1 << bits) - 1);
- addr = adis16209_addresses[chan->scan_index][0];
- return adis_write_reg_16(st, addr, val16);
- }
- return -EINVAL;
+
+ return adis_write_reg_16(st, adis16209_addresses[chan->scan_index][0],
+val & m);
}
static int adis16209_read_raw(struct iio_dev *indio_dev,
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 6/6] Staging: iio: adis16209: Move adis16209 driver out of staging
Move the adis16209 driver out of staging directory and merge to the mainline IIO subsystem. Signed-off-by: Shreeya Patel --- Changes in v6 -Move driver adis16209 from staging to mainline IIO subsystem after complete cleanup of it. drivers/iio/accel/Kconfig | 12 ++ drivers/iio/accel/Makefile| 1 + drivers/iio/accel/adis16209.c | 330 ++ drivers/staging/iio/accel/Kconfig | 12 -- drivers/staging/iio/accel/Makefile| 1 - drivers/staging/iio/accel/adis16209.c | 330 -- 6 files changed, 343 insertions(+), 343 deletions(-) create mode 100644 drivers/iio/accel/adis16209.c delete mode 100644 drivers/staging/iio/accel/adis16209.c diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index c6d9517..f95f43c 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -5,6 +5,18 @@ menu "Accelerometers" +config ADIS16209 +tristate "Analog Devices ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer" +depends on SPI +select IIO_ADIS_LIB +select IIO_ADIS_LIB_BUFFER if IIO_BUFFER +help + Say Y here to build support for Analog Devices adis16209 dual-axis digital inclinometer + and accelerometer. + + To compile this driver as a module, say M here: the module will be + called adis16209. + config ADXL345 tristate diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile index 368aedb..40861b9 100644 --- a/drivers/iio/accel/Makefile +++ b/drivers/iio/accel/Makefile @@ -4,6 +4,7 @@ # # When adding new entries keep the list in alphabetical order +obj-$(CONFIG_ADIS16209) += adis16209.o obj-$(CONFIG_ADXL345) += adxl345_core.o obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o diff --git a/drivers/iio/accel/adis16209.c b/drivers/iio/accel/adis16209.c new file mode 100644 index 000..cc50667 --- /dev/null +++ b/drivers/iio/accel/adis16209.c @@ -0,0 +1,330 @@ +/* + * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer + * + * Copyright 2010 Analog Devices Inc. + * + * Licensed under the GPL-2 or later. + */ + +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#define ADIS16209_STARTUP_DELAY_MS 220 +#define ADIS16209_FLASH_CNT_REG0x00 + +/* Data Output Register Definitions */ +#define ADIS16209_SUPPLY_OUT_REG 0x02 +#define ADIS16209_XACCL_OUT_REG0x04 +#define ADIS16209_YACCL_OUT_REG0x06 +/* Output, auxiliary ADC input */ +#define ADIS16209_AUX_ADC_REG 0x08 +/* Output, temperature */ +#define ADIS16209_TEMP_OUT_REG 0x0A +/* Output, +/- 90 degrees X-axis inclination */ +#define ADIS16209_XINCL_OUT_REG0x0C +#define ADIS16209_YINCL_OUT_REG0x0E +/* Output, +/-180 vertical rotational position */ +#define ADIS16209_ROT_OUT_REG 0x10 + +/* + * Calibration Register Definitions. + * Acceleration, inclination or rotation offset null. + */ +#define ADIS16209_XACCL_NULL_REG 0x12 +#define ADIS16209_YACCL_NULL_REG 0x14 +#define ADIS16209_XINCL_NULL_REG 0x16 +#define ADIS16209_YINCL_NULL_REG 0x18 +#define ADIS16209_ROT_NULL_REG 0x1A + +/* Alarm Register Definitions */ +#define ADIS16209_ALM_MAG1_REG 0x20 +#define ADIS16209_ALM_MAG2_REG 0x22 +#define ADIS16209_ALM_SMPL1_REG0x24 +#define ADIS16209_ALM_SMPL2_REG0x26 +#define ADIS16209_ALM_CTRL_REG 0x28 + +#define ADIS16209_AUX_DAC_REG 0x30 +#define ADIS16209_GPIO_CTRL_REG0x32 +#define ADIS16209_SMPL_PRD_REG 0x36 +#define ADIS16209_AVG_CNT_REG 0x38 +#define ADIS16209_SLP_CNT_REG 0x3A + +#define ADIS16209_MSC_CTRL_REG 0x34 +#define ADIS16209_MSC_CTRL_PWRUP_SELF_TESTBIT(10) +#define ADIS16209_MSC_CTRL_SELF_TEST_EN BIT(8) +#define ADIS16209_MSC_CTRL_DATA_RDY_ENBIT(2) +/* Data-ready polarity: 1 = active high, 0 = active low */ +#define ADIS16209_MSC_CTRL_ACTIVE_HIGHBIT(1) +#define ADIS16209_MSC_CTRL_DATA_RDY_DIO2 BIT(0) + +#define ADIS16209_STAT_REG 0x3C +#define ADIS16209_STAT_ALARM2 BIT(9) +#define ADIS16209_STAT_ALARM1 BIT(8) +#define ADIS16209_STAT_SELFTEST_FAIL_BIT 5 +#define ADIS16209_STAT_SPI_FAIL_BIT 3 +#define ADIS16209_STAT_FLASH_UPT_FAIL_BIT 2 +/* Power supply above 3.625 V */ +#define ADIS16209_STAT_POWER_HIGH_BIT 1 +/* Power supply below 3.15 V */ +#define ADIS16209_STAT_POWER_LOW_BIT 0 + +#define ADIS16209_CMD_REG 0x3E +#define ADIS16209_CMD_SW_RESETBIT(7) +#define ADIS16209_CMD_CLEAR_STAT BIT(4) +#define ADIS16209_CMD_FACTORY_CAL BIT(1) + +#define ADIS1620
Re: [PATCH 3/9] staging: ks7010: Reorder ks_wlan_netdev_ops members.
On Wed, Mar 28, 2018 at 10:51:46PM -0700, Quytelda Kahja wrote: > Reorder the members of 'ks_wlan_netdev_ops' to reflect the order > of their counterparts in the kernel's 'struct net_device_ops'. Why? This shouldn't matter at all, why make this change? greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/9] staging: ks7010: Remove trailing "_t" from all structure names.
On Wed, Mar 28, 2018 at 10:51:50PM -0700, Quytelda Kahja wrote: > The "_t" suffix is not needed for structure names in this driver, > and is a reflection of an older typedef system that is no longer > in place. Remove the "_t" suffix from every structure defined in this > driver. Again, please break this up into smaller patches, as this one does not apply to my tree at all. I've taken some patches from this series, please fix up the rest, rebase and resend the remaining ones. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: comedi: ni_stc: Fixed comment coding style issue
On Mon, Mar 26, 2018 at 03:27:10PM +0200, Rene Hickersberger wrote: > Fixed a coding style issue where the comment * was not aligned. > > Signed-off-by: Rene Hickersberger > --- > drivers/staging/comedi/drivers/ni_stc.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/comedi/drivers/ni_stc.h > b/drivers/staging/comedi/drivers/ni_stc.h > index cb9d4c3..831088c 100644 > --- a/drivers/staging/comedi/drivers/ni_stc.h > +++ b/drivers/staging/comedi/drivers/ni_stc.h > @@ -9,7 +9,7 @@ > /* > * References: > * DAQ-STC Technical Reference Manual > -*/ > + */ Does not apply to my tree at all :( ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mt7621-eth: Fix sparse warning in ethtool.c
On Wed, Mar 28, 2018 at 10:18:48PM +0100, Chris Coffey wrote: > Include the local ethtool.h header file in mtk_eth_soc.h so > implementation files have centralized access to it. > > This fixes the following sparse warning: > > drivers/staging/mt7621-eth/ethtool.c:213:6: warning: symbol > 'mtk_set_ethtool_ops' was not declared. Should it be static? > > Signed-off-by: Chris Coffey > --- > drivers/staging/mt7621-eth/mtk_eth_soc.c | 1 - > drivers/staging/mt7621-eth/mtk_eth_soc.h | 1 + > 2 files changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/mt7621-eth/mtk_eth_soc.c > b/drivers/staging/mt7621-eth/mtk_eth_soc.c > index cbc7339843..0574e71573 100644 > --- a/drivers/staging/mt7621-eth/mtk_eth_soc.c > +++ b/drivers/staging/mt7621-eth/mtk_eth_soc.c > @@ -35,7 +35,6 @@ > > #include "mtk_eth_soc.h" > #include "mdio.h" > -#include "ethtool.h" How about just moving ethtool.h above the mtk_eth_soc.h include? Putting .h file dependancies in other .h files is generally not a good idea if at all possible. Keeping them "clean" is better, and this driver has a bunch of work to go there, so let's not make it worse :) thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 07/13] staging: typec: tcpci: register port before request irq
On Thu, Mar 29, 2018 at 12:06:12AM +0800, Li Jun wrote:
> With that we can clear any pending events and the port is registered
> so driver can be ready to handle typec events once we request irq.
>
> Signed-off-by: Peter Chen
> Signed-off-by: Li Jun
These sign offs aren't clear.
Sign offs mean that you handled the patch but didn't include any of
SCO's copyrighted UNIX code into it. Normally they're in the order of
who touched the code. So Peter touched the code first. Should he get
authorship credit? How did he touch the code first if he didn't write
the code? It doesn't make sense.
> ---
> drivers/staging/typec/tcpci.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 4f7ad10..9e0014b 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -537,25 +537,26 @@ static int tcpci_probe(struct i2c_client *client,
> if (IS_ERR(chip->data.regmap))
> return PTR_ERR(chip->data.regmap);
>
> + i2c_set_clientdata(client, chip);
> +
> /* Disable chip interrupts before requesting irq */
> err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
> sizeof(u16));
> if (err < 0)
> return err;
>
> + chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> + if (PTR_ERR_OR_ZERO(chip->tcpci))
> + return PTR_ERR(chip->tcpci);
When a function returns both error pointers and NULL that means that
NULL is a secial case of success. Like for example:
p->my_feature = get_optional_feature();
If it returns NULL that means the optional feature isn't there, but it's
fine because it's optional. But if it returns an error pointer that
means the feature is there but the hardware is buggy or something so
we shouldn't continue.
If you return PTR_ERR(NULL) that means success.
I don't think this code makes sense just from looking at it and also
when I checked tcpci_register_port() doesn't return NULL.
> +
> err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> _tcpci_irq,
> IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> dev_name(&client->dev), chip);
> if (err < 0)
> - return err;
> + tcpci_unregister_port(chip->tcpci);
Can you put the "return err;" back, because that's better style. It's
better to keep the error path and success path separate if you can.
if (err < 0) {
tcpci_unregister_port(chip->tcpci);
return err;
}
return 0;
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: vt6655: check for memory allocation failures
2018-03-29 17:00 GMT+09:00 Jia-Ju Bai :
>
>
>
> On 2018/3/29 15:22, Ji-Hun Kim wrote:
>>
>> There are no null pointer checking on rd_info and td_info values which
>> are allocated by kzalloc. It has potential null pointer dereferencing
>> issues. Add return when allocation is failed.
>>
>> Signed-off-by: Ji-Hun Kim
>> ---
>>
>> Change: since v1:
>>
>> - Delete WARN_ON which can makes crashes on some machines.
>> - Instead of return directly, goto freeing function for freeing previously
>>allocated memory in the for loop after kzalloc() failed.
>> - In the freeing function, if td_info and rd_info are not allocated, no
>>needs to free.
>>
>> drivers/staging/vt6655/device_main.c | 64
>> +---
>> 1 file changed, 44 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/staging/vt6655/device_main.c
>> b/drivers/staging/vt6655/device_main.c
>> index fbc4bc6..ecbba43 100644
>> --- a/drivers/staging/vt6655/device_main.c
>> +++ b/drivers/staging/vt6655/device_main.c
>> @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private
>> *priv)
>> i ++, curr += sizeof(struct vnt_rx_desc)) {
>> desc = &priv->aRD0Ring[i];
>> desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
>> -
>> + if (!desc->rd_info)
>> + goto error;
>> if (!device_alloc_rx_buf(priv, desc))
>> dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
>> @@ -550,6 +551,10 @@ static void device_init_rd0_ring(struct vnt_private
>> *priv)
>> if (i > 0)
>> priv->aRD0Ring[i-1].next_desc =
>> cpu_to_le32(priv->rd0_pool_dma);
>> priv->pCurrRD[0] = &priv->aRD0Ring[0];
>> +
>> + return;
>> +error:
>> + device_free_rd0_ring(priv);
>> }
>>
>
>
> I think you should return an error number here, because
> device_init_rd0_ring() is called by vnt_start().
> You should also implement error handling code in vnt_start(), and let
> vnt_start() returns an error number too.
> The same for device_init_rd1_ring(), device_init_td0_ring() and
> device_init_td1_ring().
>
Hi Jia-Ju, Thanks for the feedback. All right, those function looks
that needs returns
an error number. I will implement error handling code and send a patch
v3 tomorrow
Best regards,
Ji-Hun
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] ANDROID: binder: re-order some conditions
On Thu, Mar 29, 2018 at 11:14 AM, Dan Carpenter wrote: > It doesn't make any difference to runtime but I've switched these two > checks to make my static checker happy. > > The problem is that "buffer->data_size" is user controlled and if it's > less than "sizeo(*hdr)" then that means "offset" can be more than > "buffer->data_size". It's just cleaner to check it in the other order. > > Signed-off-by: Dan Carpenter Acked-by: Martijn Coenen > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index 764b63a5aade..00322b146469 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -2058,8 +2058,8 @@ static size_t binder_validate_object(struct > binder_buffer *buffer, u64 offset) > struct binder_object_header *hdr; > size_t object_size = 0; > > - if (offset > buffer->data_size - sizeof(*hdr) || > - buffer->data_size < sizeof(*hdr) || > + if (buffer->data_size < sizeof(*hdr) || > + offset > buffer->data_size - sizeof(*hdr) || > !IS_ALIGNED(offset, sizeof(u32))) > return 0; > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: mt7621-eth: Fix sparse warning in ethtool.c
This fixes the following sparse warning:
drivers/staging/mt7621-eth/ethtool.c:213:6: warning: symbol
'mtk_set_ethtool_ops' was not declared. Should it be static?
Signed-off-by: Chris Coffey
---
Changes in v2:
- Per GregKH's feedback (thanks!), don't add unnecessary new .h file
dependencies. This patch version reverts those changes and fixes the
problem directly in ethtool.c (which is that it didn't include
ethtool.h anywhere -- mtk_set_ethtool_ops is not static).
drivers/staging/mt7621-eth/ethtool.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/mt7621-eth/ethtool.c
b/drivers/staging/mt7621-eth/ethtool.c
index 38ba0c040a..5268c5ca09 100644
--- a/drivers/staging/mt7621-eth/ethtool.c
+++ b/drivers/staging/mt7621-eth/ethtool.c
@@ -13,6 +13,7 @@
*/
#include "mtk_eth_soc.h"
+#include "ethtool.h"
static const char mtk_gdma_str[][ETH_GSTRING_LEN] = {
#define _FE(x...) # x,
--
2.11.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 04/13] usb: typec: add fwnode to tcpc
Hi,
On Thu, Mar 29, 2018 at 12:06:09AM +0800, Li Jun wrote:
> Add fwnode handle to get the fwnode so we can get typec configs
> it contains.
>
> Suggested-by: Heikki Krogerus
> Signed-off-by: Li Jun
> ---
> drivers/staging/typec/tcpci.c | 14 +++---
> drivers/usb/typec/tcpm.c | 1 +
> include/linux/usb/tcpm.h | 2 ++
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index ed76327..4f7ad10 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -463,17 +464,16 @@ static const struct regmap_config tcpci_regmap_config =
> {
> .max_register = 0x7F, /* 0x80 .. 0xFF are vendor defined */
> };
>
> -static const struct tcpc_config tcpci_tcpc_config = {
> - .type = TYPEC_PORT_DFP,
> - .default_role = TYPEC_SINK,
> -};
> -
> static int tcpci_parse_config(struct tcpci *tcpci)
> {
> tcpci->controls_vbus = true; /* XXX */
>
> - /* TODO: Populate struct tcpc_config from ACPI/device-tree */
> - tcpci->tcpc.config = &tcpci_tcpc_config;
That will break bisectablitity. tcpm.c is still accessing the config
at this point.
Just leave those untouched in here, and clean-up in separate patch
that comes after the patch that prepares tcpm.c.
Thanks,
--
heikki
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/6] staging: ks7010: convert some macros into inline functions
This patch series replace some macros with inline functions in ks_hostif.h header file and replace its uses along the source code. Sergio Paracuellos (6): staging: ks7010: replace IS_11B_RATE macro with inline function staging: ks7010: replace IS_OFDM_RATE macro with inline function staging: ks7010: replace IS_11BG_RATE macro with inline function staging: ks7010: IS_OFDM_EXT_RATE macro with inline function staging: ks7010: replace IS_HIF_IND with inline function staging: ks7010: replace IS_HIF_CONF with inline function drivers/staging/ks7010/ks7010_sdio.c | 2 +- drivers/staging/ks7010/ks_hostif.c | 6 ++-- drivers/staging/ks7010/ks_hostif.h | 70 3 files changed, 52 insertions(+), 26 deletions(-) -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 5/6] staging: ks7010: replace IS_HIF_IND with inline function
This commit replaces IS_HIF_IND macro with is_11b_rate inline
function to improve readability.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/ks7010/ks_hostif.h | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.h
b/drivers/staging/ks7010/ks_hostif.h
index 10c8f09..24482c5 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -610,17 +610,21 @@ enum multicast_filter_type {
/* macro function */
#define HIF_EVENT_MASK 0xE800
-#define IS_HIF_IND(_EVENT) ((_EVENT & HIF_EVENT_MASK) == 0xE800 && \
-((_EVENT & ~HIF_EVENT_MASK) == 0x0001 || \
-(_EVENT & ~HIF_EVENT_MASK) == 0x0006 || \
-(_EVENT & ~HIF_EVENT_MASK) == 0x000C || \
-(_EVENT & ~HIF_EVENT_MASK) == 0x0011 || \
-(_EVENT & ~HIF_EVENT_MASK) == 0x0012))
+
+static inline bool is_hif_ind(unsigned short event)
+{
+ return (((event & HIF_EVENT_MASK) == HIF_EVENT_MASK) &&
+ (((event & ~HIF_EVENT_MASK) == 0x0001) ||
+((event & ~HIF_EVENT_MASK) == 0x0006) ||
+((event & ~HIF_EVENT_MASK) == 0x000C) ||
+((event & ~HIF_EVENT_MASK) == 0x0011) ||
+((event & ~HIF_EVENT_MASK) == 0x0012)));
+}
#define IS_HIF_CONF(_EVENT) ((_EVENT & HIF_EVENT_MASK) == 0xE800 && \
(_EVENT & ~HIF_EVENT_MASK) > 0x && \
(_EVENT & ~HIF_EVENT_MASK) < 0x0012 && \
-!IS_HIF_IND(_EVENT))
+!is_hif_ind(_EVENT))
#ifdef __KERNEL__
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 6/6] staging: ks7010: replace IS_HIF_CONF with inline function
This commit replaces IS_HIF_CONF macro with is_11b_rate inline
function to improve readability.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/ks7010/ks7010_sdio.c | 2 +-
drivers/staging/ks7010/ks_hostif.h | 12 +++-
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/ks7010/ks7010_sdio.c
b/drivers/staging/ks7010/ks7010_sdio.c
index b8f55a1..0cc14ac 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -408,7 +408,7 @@ static void ks_wlan_hw_rx(struct ks_wlan_private *priv,
uint16_t size)
netdev_err(priv->net_dev, " error : READ_STATUS\n");
if (atomic_read(&priv->psstatus.confirm_wait)) {
- if (IS_HIF_CONF(event)) {
+ if (is_hif_conf(event)) {
netdev_dbg(priv->net_dev, "IS_HIF_CONF true !!\n");
atomic_dec(&priv->psstatus.confirm_wait);
}
diff --git a/drivers/staging/ks7010/ks_hostif.h
b/drivers/staging/ks7010/ks_hostif.h
index 24482c5..b785f62 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -608,7 +608,6 @@ enum multicast_filter_type {
#define NIC_MAX_MCAST_LIST 32
-/* macro function */
#define HIF_EVENT_MASK 0xE800
static inline bool is_hif_ind(unsigned short event)
@@ -621,10 +620,13 @@ static inline bool is_hif_ind(unsigned short event)
((event & ~HIF_EVENT_MASK) == 0x0012)));
}
-#define IS_HIF_CONF(_EVENT) ((_EVENT & HIF_EVENT_MASK) == 0xE800 && \
-(_EVENT & ~HIF_EVENT_MASK) > 0x && \
-(_EVENT & ~HIF_EVENT_MASK) < 0x0012 && \
-!is_hif_ind(_EVENT))
+static inline bool is_hif_conf(unsigned short event)
+{
+ return (((event & HIF_EVENT_MASK) == HIF_EVENT_MASK) &&
+ ((event & ~HIF_EVENT_MASK) > 0x) &&
+ ((event & ~HIF_EVENT_MASK) < 0x0012) &&
+ !is_hif_ind(event));
+}
#ifdef __KERNEL__
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/6] staging: ks7010: IS_OFDM_EXT_RATE macro with inline function
This commit replaces IS_OFDM_EXT_RATE macro with is_11b_rate
inline function to improve readability.
It also fix a checkpatch script warning because a line with more
than 80 spaces.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/ks7010/ks_hostif.c | 2 +-
drivers/staging/ks7010/ks_hostif.h | 11 ---
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index cca2d57..e336d25 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -1900,7 +1900,7 @@ void hostif_sme_mode_setup(struct ks_wlan_private *priv)
if (!is_11bg_rate(priv->reg.rate_set.body[i]))
break;
- if (IS_OFDM_EXT_RATE(priv->reg.rate_set.body[i])) {
+ if (is_ofdm_ext_rate(priv->reg.rate_set.body[i])) {
rate_octet[i] = priv->reg.rate_set.body[i] &
RATE_MASK;
} else {
diff --git a/drivers/staging/ks7010/ks_hostif.h
b/drivers/staging/ks7010/ks_hostif.h
index 46d8d26..10c8f09 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -581,9 +581,14 @@ static inline bool is_11bg_rate(u8 rate)
return (is_11b_rate(rate) || is_ofdm_rate(rate));
}
-#define IS_OFDM_EXT_RATE(A) (((A & RATE_MASK) == TX_RATE_9M) || ((A &
RATE_MASK) == TX_RATE_18M) || \
-((A & RATE_MASK) == TX_RATE_36M) || ((A &
RATE_MASK) == TX_RATE_48M) || \
-((A & RATE_MASK) == TX_RATE_54M))
+static inline bool is_ofdm_ext_rate(u8 rate)
+{
+ return (((rate & RATE_MASK) == TX_RATE_9M) ||
+ ((rate & RATE_MASK) == TX_RATE_18M) ||
+ ((rate & RATE_MASK) == TX_RATE_36M) ||
+ ((rate & RATE_MASK) == TX_RATE_48M) ||
+ ((rate & RATE_MASK) == TX_RATE_54M));
+}
enum connect_status_type {
CONNECT_STATUS,
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/6] staging: ks7010: replace IS_11BG_RATE macro with inline function
This commit replaces IS_11BG_RATE macro with is_11b_rate inline
function to improve readability.
It also fix a checkpatch script warning because a line with more
than 80 spaces.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/ks7010/ks_hostif.c | 2 +-
drivers/staging/ks7010/ks_hostif.h | 5 -
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index 9f6d49e..cca2d57 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -1897,7 +1897,7 @@ void hostif_sme_mode_setup(struct ks_wlan_private *priv)
} else {/* D_11G_ONLY_MODE or D_11BG_COMPATIBLE_MODE */
for (i = 0; i < priv->reg.rate_set.size; i++) {
- if (!IS_11BG_RATE(priv->reg.rate_set.body[i]))
+ if (!is_11bg_rate(priv->reg.rate_set.body[i]))
break;
if (IS_OFDM_EXT_RATE(priv->reg.rate_set.body[i])) {
diff --git a/drivers/staging/ks7010/ks_hostif.h
b/drivers/staging/ks7010/ks_hostif.h
index 04bff23..46d8d26 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -576,7 +576,10 @@ static inline bool is_ofdm_rate(u8 rate)
((rate & RATE_MASK) == TX_RATE_54M));
}
-#define IS_11BG_RATE(A) (is_11b_rate(A) || is_ofdm_rate(A))
+static inline bool is_11bg_rate(u8 rate)
+{
+ return (is_11b_rate(rate) || is_ofdm_rate(rate));
+}
#define IS_OFDM_EXT_RATE(A) (((A & RATE_MASK) == TX_RATE_9M) || ((A &
RATE_MASK) == TX_RATE_18M) || \
((A & RATE_MASK) == TX_RATE_36M) || ((A &
RATE_MASK) == TX_RATE_48M) || \
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/6] staging: ks7010: replace IS_11B_RATE macro with inline function
This commit replaces IS_11B_RATE macro with is_11b_rate inline
function to improve readability.
It also fix a checkpatch script warning because a line with more
than 80 spaces.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/ks7010/ks_hostif.c | 2 +-
drivers/staging/ks7010/ks_hostif.h | 11 ---
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index 676961c..9f6d49e 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -1884,7 +1884,7 @@ void hostif_sme_mode_setup(struct ks_wlan_private *priv)
/* rate mask by phy setting */
if (priv->reg.phy_type == D_11B_ONLY_MODE) {
for (i = 0; i < priv->reg.rate_set.size; i++) {
- if (!IS_11B_RATE(priv->reg.rate_set.body[i]))
+ if (!is_11b_rate(priv->reg.rate_set.body[i]))
break;
if ((priv->reg.rate_set.body[i] & RATE_MASK) >=
TX_RATE_5M) {
diff --git a/drivers/staging/ks7010/ks_hostif.h
b/drivers/staging/ks7010/ks_hostif.h
index 2f918b1..ae2adbf 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -556,15 +556,20 @@ struct hostif_mic_failure_confirm_t {
#define TX_RATE_48M(uint8_t)(480 / 5)
#define TX_RATE_54M(uint8_t)(540 / 5)
-#define IS_11B_RATE(A) (((A & RATE_MASK) == TX_RATE_1M) || ((A & RATE_MASK) ==
TX_RATE_2M) || \
- ((A & RATE_MASK) == TX_RATE_5M) || ((A & RATE_MASK) ==
TX_RATE_11M))
+static inline bool is_11b_rate(u8 rate)
+{
+ return (((rate & RATE_MASK) == TX_RATE_1M) ||
+ ((rate & RATE_MASK) == TX_RATE_2M) ||
+ ((rate & RATE_MASK) == TX_RATE_5M) ||
+ ((rate & RATE_MASK) == TX_RATE_11M));
+}
#define IS_OFDM_RATE(A) (((A & RATE_MASK) == TX_RATE_6M) || ((A & RATE_MASK)
== TX_RATE_12M) || \
((A & RATE_MASK) == TX_RATE_24M) || ((A & RATE_MASK)
== TX_RATE_9M) || \
((A & RATE_MASK) == TX_RATE_18M) || ((A & RATE_MASK)
== TX_RATE_36M) || \
((A & RATE_MASK) == TX_RATE_48M) || ((A & RATE_MASK)
== TX_RATE_54M))
-#define IS_11BG_RATE(A) (IS_11B_RATE(A) || IS_OFDM_RATE(A))
+#define IS_11BG_RATE(A) (is_11b_rate(A) || IS_OFDM_RATE(A))
#define IS_OFDM_EXT_RATE(A) (((A & RATE_MASK) == TX_RATE_9M) || ((A &
RATE_MASK) == TX_RATE_18M) || \
((A & RATE_MASK) == TX_RATE_36M) || ((A &
RATE_MASK) == TX_RATE_48M) || \
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/6] staging: ks7010: replace IS_OFDM_RATE macro with inline function
This commit replaces IS_OFDM_RATE macro with is_ofdm_rate inline
function.
This also fix checkpatch warning about more than 80 spaces line.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/ks7010/ks_hostif.h | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.h
b/drivers/staging/ks7010/ks_hostif.h
index ae2adbf..04bff23 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -564,12 +564,19 @@ static inline bool is_11b_rate(u8 rate)
((rate & RATE_MASK) == TX_RATE_11M));
}
-#define IS_OFDM_RATE(A) (((A & RATE_MASK) == TX_RATE_6M) || ((A & RATE_MASK)
== TX_RATE_12M) || \
-((A & RATE_MASK) == TX_RATE_24M) || ((A & RATE_MASK)
== TX_RATE_9M) || \
-((A & RATE_MASK) == TX_RATE_18M) || ((A & RATE_MASK)
== TX_RATE_36M) || \
-((A & RATE_MASK) == TX_RATE_48M) || ((A & RATE_MASK)
== TX_RATE_54M))
+static inline bool is_ofdm_rate(u8 rate)
+{
+ return (((rate & RATE_MASK) == TX_RATE_6M) ||
+ ((rate & RATE_MASK) == TX_RATE_12M) ||
+ ((rate & RATE_MASK) == TX_RATE_24M) ||
+ ((rate & RATE_MASK) == TX_RATE_9M) ||
+ ((rate & RATE_MASK) == TX_RATE_18M) ||
+ ((rate & RATE_MASK) == TX_RATE_36M) ||
+ ((rate & RATE_MASK) == TX_RATE_48M) ||
+ ((rate & RATE_MASK) == TX_RATE_54M));
+}
-#define IS_11BG_RATE(A) (is_11b_rate(A) || IS_OFDM_RATE(A))
+#define IS_11BG_RATE(A) (is_11b_rate(A) || is_ofdm_rate(A))
#define IS_OFDM_EXT_RATE(A) (((A & RATE_MASK) == TX_RATE_9M) || ((A &
RATE_MASK) == TX_RATE_18M) || \
((A & RATE_MASK) == TX_RATE_36M) || ((A &
RATE_MASK) == TX_RATE_48M) || \
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 01/13] dt-bindings: connector: add properties for typec
Hi Li,
On 03/28/2018 06:06 PM, Li Jun wrote:
> Add bingdings supported by current typec driver, so user can pass
> all those properties via dt.
>
> Signed-off-by: Li Jun
> ---
> .../bindings/connector/usb-connector.txt | 39
> ++
> 1 file changed, 39 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt
> b/Documentation/devicetree/bindings/connector/usb-connector.txt
> index e1463f1..922f22b 100644
> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -15,6 +15,29 @@ Optional properties:
> - type: size of the connector, should be specified in case of USB-A, USB-B
>non-fullsize connectors: "mini", "micro".
>
> +Optional properties for usb-c-connector:
> +- power-type: should be one of "source", "sink" or "dual"(DRP) if typec
> + connector has power support.
> +- try-power-role: preferred power role if "dual"(DRP) can support Try.SNK
> + or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> +- data-type: should be one of "host", "device", "dual"(DRD) if typec
> + connector supports USB data.
> +
> +Required properties for usb-c-connector with power delivery support:
> +- source-pdos: An array of u32 with each entry providing supported power
> + source data object(PDO), the detailed bit definitions of PDO can be found
> + in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
> + Source_Capabilities Message, the order of each entry(PDO) should follow
> + the PD spec chapter 6.4.1. Required for power source and power dual role.
> +- sink-pdos: An array of u32 with each entry providing supported power
> + sink data object(PDO), the detailed bit definitions of PDO can be found
> + in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
> + Sink Capabilities Message, the order of each entry(PDO) should follow
> + the PD spec chapter 6.4.1. Required for power sink and power dual role.
> +- op-sink-microwatt-hours: Sink required operating power in micro
> + watt-hours, if source offered power is less then it, Capability Mismatch
> + is set, required for power sink and power dual role.
This doesn't make sense. The unit of power is watt (W), watt-hour on the other
hand
is a measurement of energy. I think "op-sink-microwatt" is what we want here.
// Mats
> +
> Required nodes:
> - any data bus to the connector should be modeled using the OF graph bindings
>specified in bindings/graph.txt, unless the bus is between parent node and
> @@ -73,3 +96,19 @@ ccic: s2mm005@33 {
> };
> };
> };
> +
> +3. USB-C connector attached to a typec port controller(ptn5110), which has
> +power delivery support and enables drp.
> +
> +typec: ptn5110@50 {
> + ...
> + usb_con: connector {
> + compatible = "usb-c-connector";
> + label = "USB-C";
> + power-type = "dual";
> + try-power-role = "sink";
> + source-pdos = <0x380190c8>;
> + sink-pdos = <0x380190c8 0x3802d0c8>;
> + op-sink-microwatt-hours = <900>;
> + };
> +};
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach
Hi Li, On 03/28/2018 06:06 PM, Li Jun wrote: > In case of drp toggling, we may need set correct cc value for role control > after attach as it may never been set. > > Signed-off-by: Li Jun > --- > drivers/usb/typec/tcpm.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > index 218c230..72d4232 100644 > --- a/drivers/usb/typec/tcpm.c > +++ b/drivers/usb/typec/tcpm.c > @@ -2126,6 +2126,7 @@ static void tcpm_reset_port(struct tcpm_port *port) > tcpm_set_attached_state(port, false); > port->try_src_count = 0; > port->try_snk_count = 0; > + port->cc_req = 0; I don't think it's OK to use "0" here. cc_req is an enum so why not use "|TYPEC_CC_OPEN"?| > } > > static void tcpm_detach(struct tcpm_port *port) > @@ -2361,6 +2362,8 @@ static void run_state_machine(struct tcpm_port *port) > break; > > case SRC_ATTACHED: > + if (!port->cc_req) if (port->cc_req == |TYPEC_CC_OPEN)| > + tcpm_set_cc(port, tcpm_rp_cc(port)); > ret = tcpm_src_attach(port); > tcpm_set_state(port, SRC_UNATTACHED, > ret < 0 ? 0 : PD_T_PS_SOURCE_ON); > @@ -2531,6 +2534,8 @@ static void run_state_machine(struct tcpm_port *port) > tcpm_set_state(port, SNK_UNATTACHED, PD_T_PD_DEBOUNCE); > break; > case SNK_ATTACHED: > + if (!port->cc_req) Ditto. > + tcpm_set_cc(port, TYPEC_CC_RD); > ret = tcpm_snk_attach(port); > if (ret < 0) > tcpm_set_state(port, SNK_UNATTACHED, 0); // Mats ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach
On Thu, Mar 29, 2018 at 12:06:15AM +0800, Li Jun wrote: > In case of drp toggling, we may need set correct cc value for role control > after attach as it may never been set. > Isn't CC set by the lower level driver in this case ? In other words, is it ever necessary to call back into the low level driver to set CC again ? Doing that in attached state seems a bit late. It may make more sense to update port->cc_req when the state machine leaves DRP_TOGGLING state, ie in _tcpm_cc_change(), and to do it without callback into the low level driver (it should not be necessary). Guenter > Signed-off-by: Li Jun > --- > drivers/usb/typec/tcpm.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > index 218c230..72d4232 100644 > --- a/drivers/usb/typec/tcpm.c > +++ b/drivers/usb/typec/tcpm.c > @@ -2126,6 +2126,7 @@ static void tcpm_reset_port(struct tcpm_port *port) > tcpm_set_attached_state(port, false); > port->try_src_count = 0; > port->try_snk_count = 0; > + port->cc_req = 0; > } > > static void tcpm_detach(struct tcpm_port *port) > @@ -2361,6 +2362,8 @@ static void run_state_machine(struct tcpm_port *port) > break; > > case SRC_ATTACHED: > + if (!port->cc_req) > + tcpm_set_cc(port, tcpm_rp_cc(port)); > ret = tcpm_src_attach(port); > tcpm_set_state(port, SRC_UNATTACHED, > ret < 0 ? 0 : PD_T_PS_SOURCE_ON); > @@ -2531,6 +2534,8 @@ static void run_state_machine(struct tcpm_port *port) > tcpm_set_state(port, SNK_UNATTACHED, PD_T_PD_DEBOUNCE); > break; > case SNK_ATTACHED: > + if (!port->cc_req) > + tcpm_set_cc(port, TYPEC_CC_RD); > ret = tcpm_snk_attach(port); > if (ret < 0) > tcpm_set_state(port, SNK_UNATTACHED, 0); > -- > 2.7.4 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] staging: vt6655: check for memory allocation failures
There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Implement error handling code on device_init_rd*, device_init_td*
and vnt_start for the allocation failures.
Signed-off-by: Ji-Hun Kim
---
Changes v2:
- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
allocated memory in the for loop after kzalloc() failed.
- In the freeing function, add if statement for freeing to only allocated
values.
Changes v3:
- Modify return type of device_init_rd*, device_init_td*. Then add returns
error code at those functions and vnt_start as well.
drivers/staging/vt6655/device_main.c | 114 +--
1 file changed, 82 insertions(+), 32 deletions(-)
diff --git a/drivers/staging/vt6655/device_main.c
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..0d55f34 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -124,10 +124,10 @@
static void device_free_info(struct vnt_private *priv);
static void device_print_info(struct vnt_private *priv);
-static void device_init_rd0_ring(struct vnt_private *priv);
-static void device_init_rd1_ring(struct vnt_private *priv);
-static void device_init_td0_ring(struct vnt_private *priv);
-static void device_init_td1_ring(struct vnt_private *priv);
+static int device_init_rd0_ring(struct vnt_private *priv);
+static int device_init_rd1_ring(struct vnt_private *priv);
+static int device_init_td0_ring(struct vnt_private *priv);
+static int device_init_td1_ring(struct vnt_private *priv);
static int device_rx_srv(struct vnt_private *priv, unsigned int idx);
static int device_tx_srv(struct vnt_private *priv, unsigned int idx);
@@ -528,18 +528,22 @@ static void device_free_rings(struct vnt_private *priv)
priv->tx0_bufs, priv->tx_bufs_dma0);
}
-static void device_init_rd0_ring(struct vnt_private *priv)
+static int device_init_rd0_ring(struct vnt_private *priv)
{
int i;
dma_addr_t curr = priv->rd0_pool_dma;
struct vnt_rx_desc *desc;
+ int ret = 0;
/* Init the RD0 ring entries */
for (i = 0; i < priv->opts.rx_descs0;
i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = &priv->aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+ if (!desc->rd_info) {
+ ret = -ENOMEM;
+ goto error;
+ }
if (!device_alloc_rx_buf(priv, desc))
dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = &priv->aRD0Ring[0];
+
+ return 0;
+error:
+ device_free_rd0_ring(priv);
+ return ret;
}
-static void device_init_rd1_ring(struct vnt_private *priv)
+static int device_init_rd1_ring(struct vnt_private *priv)
{
int i;
dma_addr_t curr = priv->rd1_pool_dma;
struct vnt_rx_desc *desc;
+ int ret = 0;
/* Init the RD1 ring entries */
for (i = 0; i < priv->opts.rx_descs1;
i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = &priv->aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+ if (!desc->rd_info) {
+ ret = -ENOMEM;
+ goto error;
+ }
if (!device_alloc_rx_buf(priv, desc))
dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
@@ -574,6 +587,11 @@ static void device_init_rd1_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
priv->pCurrRD[1] = &priv->aRD1Ring[0];
+
+ return 0;
+error:
+ device_free_rd1_ring(priv);
+ return ret;
}
static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +602,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = &priv->aRD0Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
- dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
- dev_kfree_skb(rd_info->skb);
-
- kfree(desc->rd_info);
+ if (rd_info) {
+ dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
+ priv->rx_buf_sz, DMA_FROM_DEVICE);
+ dev_kfree_skb(rd_info->skb);
+ kfree(desc->rd_info);
+ }
}
}
@@ -601,27 +619,31 @@ st
Re: [PATCH v3] staging: vt6655: check for memory allocation failures
On 2018/3/30 10:44, Ji-Hun Kim wrote: @@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw) } dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n"); - device_init_rd0_ring(priv); - device_init_rd1_ring(priv); - device_init_td0_ring(priv); - device_init_td1_ring(priv); + ret = device_init_rd0_ring(priv); + if (ret) + goto error; + ret = device_init_rd1_ring(priv); + if (ret) + goto error; + ret = device_init_td0_ring(priv); + if (ret) + goto error; + ret = device_init_td1_ring(priv); + if (ret) + goto error; device_init_registers(priv); @@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw) ieee80211_wake_queues(hw); return 0; +error: + return ret; } This code will lead to memory leaks when device_init_rd1_ring() fails, because the memory allocated by device_init_rd0_ring() is not freed. I think this one will be better: ret = device_init_rd0_ring(priv); if (ret) goto error_init_rd0_ring; ret = device_init_rd1_ring(priv); if (ret) goto error_init_rd1_ring; ret = device_init_td0_ring(priv); if (ret) goto error_init_td0_ring; ret = device_init_td1_ring(priv); if (ret) goto error_init_td1_ring; .. error_init_td1_ring: device_free_td0_ring(priv); error_init_td0_ring: device_free_rd1_ring(priv); error_init_rd1_ring: device_free_rd0_ring(priv); error_init_rd0_ring: return ret; Best wishes, Jia-Ju Bai ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: vt6655: check for memory allocation failures
On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote: > > > On 2018/3/30 10:44, Ji-Hun Kim wrote: > >@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw) > > } > > dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n"); > >-device_init_rd0_ring(priv); > >-device_init_rd1_ring(priv); > >-device_init_td0_ring(priv); > >-device_init_td1_ring(priv); > >+ret = device_init_rd0_ring(priv); > >+if (ret) > >+goto error; > >+ret = device_init_rd1_ring(priv); > >+if (ret) > >+goto error; > >+ret = device_init_td0_ring(priv); > >+if (ret) > >+goto error; > >+ret = device_init_td1_ring(priv); > >+if (ret) > >+goto error; > > device_init_registers(priv); > >@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw) > > ieee80211_wake_queues(hw); > > return 0; > >+error: > >+return ret; > > } > > This code will lead to memory leaks when device_init_rd1_ring() > fails, because the memory allocated by device_init_rd0_ring() is not > freed. > > I think this one will be better: > ret = device_init_rd0_ring(priv); > if (ret) > goto error_init_rd0_ring; > ret = device_init_rd1_ring(priv); > if (ret) > goto error_init_rd1_ring; > ret = device_init_td0_ring(priv); > if (ret) > goto error_init_td0_ring; > ret = device_init_td1_ring(priv); > if (ret) > goto error_init_td1_ring; > .. > error_init_td1_ring: > device_free_td0_ring(priv); > error_init_td0_ring: > device_free_rd1_ring(priv); > error_init_rd1_ring: > device_free_rd0_ring(priv); > error_init_rd0_ring: > return ret; > > > Best wishes, > Jia-Ju Bai > > But, those freeing function are already placed in the each device_init functions for allocation fail like below. @@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv) + return 0; +error: + device_free_rd0_ring(priv); + return ret; Would freeing in the vnt_start() be better instead of freeing in device_init_rd0_ring function? Best regards, Ji-Hun ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: vt6655: check for memory allocation failures
On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote: > > > On 2018/3/30 10:44, Ji-Hun Kim wrote: > >@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw) > > } > > dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n"); > >-device_init_rd0_ring(priv); > >-device_init_rd1_ring(priv); > >-device_init_td0_ring(priv); > >-device_init_td1_ring(priv); > >+ret = device_init_rd0_ring(priv); > >+if (ret) > >+goto error; > >+ret = device_init_rd1_ring(priv); > >+if (ret) > >+goto error; > >+ret = device_init_td0_ring(priv); > >+if (ret) > >+goto error; > >+ret = device_init_td1_ring(priv); > >+if (ret) > >+goto error; > > device_init_registers(priv); > >@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw) > > ieee80211_wake_queues(hw); > > return 0; > >+error: > >+return ret; > > } > > This code will lead to memory leaks when device_init_rd1_ring() > fails, because the memory allocated by device_init_rd0_ring() is not > freed. > > I think this one will be better: > ret = device_init_rd0_ring(priv); > if (ret) > goto error_init_rd0_ring; > ret = device_init_rd1_ring(priv); > if (ret) > goto error_init_rd1_ring; > ret = device_init_td0_ring(priv); > if (ret) > goto error_init_td0_ring; > ret = device_init_td1_ring(priv); > if (ret) > goto error_init_td1_ring; > .. > error_init_td1_ring: > device_free_td0_ring(priv); > error_init_td0_ring: > device_free_rd1_ring(priv); > error_init_rd1_ring: > device_free_rd0_ring(priv); > error_init_rd0_ring: > return ret; > > > Best wishes, > Jia-Ju Bai > > Oh, sorry, I got what you said. Yes, you are right. I am going to make patch v4. Thanks! Best regards, Ji-Hun ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: vt6655: check for memory allocation failures
On 2018/3/30 11:39, Ji-Hun Kim wrote: On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote: On 2018/3/30 10:44, Ji-Hun Kim wrote: @@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw) } dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n"); - device_init_rd0_ring(priv); - device_init_rd1_ring(priv); - device_init_td0_ring(priv); - device_init_td1_ring(priv); + ret = device_init_rd0_ring(priv); + if (ret) + goto error; + ret = device_init_rd1_ring(priv); + if (ret) + goto error; + ret = device_init_td0_ring(priv); + if (ret) + goto error; + ret = device_init_td1_ring(priv); + if (ret) + goto error; device_init_registers(priv); @@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw) ieee80211_wake_queues(hw); return 0; +error: + return ret; } This code will lead to memory leaks when device_init_rd1_ring() fails, because the memory allocated by device_init_rd0_ring() is not freed. I think this one will be better: ret = device_init_rd0_ring(priv); if (ret) goto error_init_rd0_ring; ret = device_init_rd1_ring(priv); if (ret) goto error_init_rd1_ring; ret = device_init_td0_ring(priv); if (ret) goto error_init_td0_ring; ret = device_init_td1_ring(priv); if (ret) goto error_init_td1_ring; .. error_init_td1_ring: device_free_td0_ring(priv); error_init_td0_ring: device_free_rd1_ring(priv); error_init_rd1_ring: device_free_rd0_ring(priv); error_init_rd0_ring: return ret; Best wishes, Jia-Ju Bai But, those freeing function are already placed in the each device_init functions for allocation fail like below. I think it is okay. I suppose that, for each device_init function, you only free the resources allocated in this function, and do not handle the resources in other functions. @@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv) + return 0; +error: + device_free_rd0_ring(priv); + return ret; This code is needed to free the resources allocated in this function. Best wishes, Jia-Ju Bai ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 1/2] staging: vt6655: check for memory allocation failures
There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Implement error handling code on device_init_rd*, device_init_td*
and vnt_start for the allocation failures.
Signed-off-by: Ji-Hun Kim
---
Changes v2:
- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
allocated memory in the for loop after kzalloc() failed.
- In the freeing function, add if statement for freeing to only allocated
values.
Changes v3:
- Modify return type of device_init_rd*, device_init_td*. Then add returns
error code at those functions and vnt_start as well.
Changes v4:
- Fix potential memory leaks from error handling code of device init
functions in vnt_start().
drivers/staging/vt6655/device_main.c | 121 ++-
1 file changed, 89 insertions(+), 32 deletions(-)
diff --git a/drivers/staging/vt6655/device_main.c
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..c9752df 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -124,10 +124,10 @@
static void device_free_info(struct vnt_private *priv);
static void device_print_info(struct vnt_private *priv);
-static void device_init_rd0_ring(struct vnt_private *priv);
-static void device_init_rd1_ring(struct vnt_private *priv);
-static void device_init_td0_ring(struct vnt_private *priv);
-static void device_init_td1_ring(struct vnt_private *priv);
+static int device_init_rd0_ring(struct vnt_private *priv);
+static int device_init_rd1_ring(struct vnt_private *priv);
+static int device_init_td0_ring(struct vnt_private *priv);
+static int device_init_td1_ring(struct vnt_private *priv);
static int device_rx_srv(struct vnt_private *priv, unsigned int idx);
static int device_tx_srv(struct vnt_private *priv, unsigned int idx);
@@ -528,18 +528,22 @@ static void device_free_rings(struct vnt_private *priv)
priv->tx0_bufs, priv->tx_bufs_dma0);
}
-static void device_init_rd0_ring(struct vnt_private *priv)
+static int device_init_rd0_ring(struct vnt_private *priv)
{
int i;
dma_addr_t curr = priv->rd0_pool_dma;
struct vnt_rx_desc *desc;
+ int ret = 0;
/* Init the RD0 ring entries */
for (i = 0; i < priv->opts.rx_descs0;
i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = &priv->aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+ if (!desc->rd_info) {
+ ret = -ENOMEM;
+ goto error;
+ }
if (!device_alloc_rx_buf(priv, desc))
dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = &priv->aRD0Ring[0];
+
+ return 0;
+error:
+ device_free_rd0_ring(priv);
+ return ret;
}
-static void device_init_rd1_ring(struct vnt_private *priv)
+static int device_init_rd1_ring(struct vnt_private *priv)
{
int i;
dma_addr_t curr = priv->rd1_pool_dma;
struct vnt_rx_desc *desc;
+ int ret = 0;
/* Init the RD1 ring entries */
for (i = 0; i < priv->opts.rx_descs1;
i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = &priv->aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+ if (!desc->rd_info) {
+ ret = -ENOMEM;
+ goto error;
+ }
if (!device_alloc_rx_buf(priv, desc))
dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
@@ -574,6 +587,11 @@ static void device_init_rd1_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
priv->pCurrRD[1] = &priv->aRD1Ring[0];
+
+ return 0;
+error:
+ device_free_rd1_ring(priv);
+ return ret;
}
static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +602,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = &priv->aRD0Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
- dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
- dev_kfree_skb(rd_info->skb);
-
- kfree(desc->rd_info);
+ if (rd_info) {
+ dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
+ priv->rx_buf_sz, DMA_FROM_DEVICE);
+ dev_kfree_skb(rd_info->s
[PATCH v4 2/2] staging: vt6655: add handling memory leak on vnt_start()
There was no code for handling memory leaks of device_init_rings() and
request_irq(). It needs to free allocated memory in the device_init_rings()
, when request_irq() is failed. Add freeing sequences of irq and device
init rings.
Signed-off-by: Ji-Hun Kim
---
It's additional memory leak handling patch from
[PATCH v4 1/2] staging: vt6655: check for memory allocation failures
drivers/staging/vt6655/device_main.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/vt6655/device_main.c
b/drivers/staging/vt6655/device_main.c
index c9752df..3604f2d 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -1194,14 +1194,17 @@ static int vnt_start(struct ieee80211_hw *hw)
int ret;
priv->rx_buf_sz = PKT_BUF_SZ;
- if (!device_init_rings(priv))
- return -ENOMEM;
+ ret = (int)device_init_rings(priv);
+ if (!ret) {
+ ret = -ENOMEM;
+ goto err_init_rings;
+ }
ret = request_irq(priv->pcid->irq, vnt_interrupt,
IRQF_SHARED, "vt6655", priv);
if (ret) {
dev_dbg(&priv->pcid->dev, "failed to start irq\n");
- return ret;
+ goto err_irq;
}
dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n");
@@ -1234,6 +1237,10 @@ static int vnt_start(struct ieee80211_hw *hw)
err_init_rd1_ring:
device_free_rd0_ring(priv);
err_init_rd0_ring:
+ free_irq(priv->pcid->irq, priv);
+err_irq:
+ device_free_rings(priv);
+err_init_rings:
return ret;
}
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/9] staging: ks7010: Replace manual array copy with ether_addr_copy().
If we just want to have the address field full of zeros during the driver probe, is there a reason we should zero it explicitly here? When the 'struct net_device' is first allocated using alloc_etherdev(), the dev_addr field is explicitly set to zeros (in the subfunction dev_addr_init() in net/core/dev_addr_lists.c), so it should be zeroed already at this point in the code. Thank you, Quytelda Kahja On Wed, Mar 28, 2018 at 11:02 PM, Joe Perches wrote: > On Wed, 2018-03-28 at 22:51 -0700, Quytelda Kahja wrote: >> Copying the dummy HW address into the struct net_device doesn't need >> to be done byte by byte; use ether_addr_copy() instead. >> Additionally, dev->dev_addr is not eight bytes long. >> ether_setup() sets the dev->addr_len to ETH_ALEN (defined as 6) >> in the net core code. > [] >> diff --git a/drivers/staging/ks7010/ks_wlan_net.c >> b/drivers/staging/ks7010/ks_wlan_net.c > [] >> @@ -2900,15 +2900,7 @@ int ks_wlan_net_start(struct net_device *dev) >> timer_setup(&update_phyinfo_timer, ks_wlan_update_phyinfo_timeout, 0); >> >> /* dummy address set */ >> - memcpy(priv->eth_addr, dummy_addr, ETH_ALEN); > > why remove the copy of dummy_addr into priv->eth_addr ? > > Also, dummy_addr could be removed and eth_zero_addr() > used instead. > >> - dev->dev_addr[0] = priv->eth_addr[0]; >> - dev->dev_addr[1] = priv->eth_addr[1]; >> - dev->dev_addr[2] = priv->eth_addr[2]; >> - dev->dev_addr[3] = priv->eth_addr[3]; >> - dev->dev_addr[4] = priv->eth_addr[4]; >> - dev->dev_addr[5] = priv->eth_addr[5]; >> - dev->dev_addr[6] = 0x00; >> - dev->dev_addr[7] = 0x00; >> + ether_addr_copy(dev->dev_addr, priv->eth_addr); > > Perhaps > > eth_zero_addr(priv->eth_addr); > eth_zero_addr(dev->dev_addr); > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/9] staging: ks7010: Replace manual array copy with ether_addr_copy().
On Thu, 2018-03-29 at 23:03 -0700, Quytelda Kahja wrote: > If we just want to have the address field full of zeros during the > driver probe, is there a reason we should zero it explicitly here? > When the 'struct net_device' is first allocated using > alloc_etherdev(), the dev_addr field is explicitly set to zeros (in > the subfunction dev_addr_init() in net/core/dev_addr_lists.c), so it > should be zeroed already at this point in the code. You didn't describe the removal in your commit message so it looked unintentional. You should describe _why_ it's OK to remove it in that commit message. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
