Re: [PATCH 2/3 v2] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type
On Mon, 26 Jan 2015, Tomi Valkeinen wrote: > Hi, > > On 25/01/15 16:47, Nicholas Mc Guire wrote: > > Signed-off-by: Nicholas Mc Guire > > --- > > > > v2: fixed subject line > > > > The return type of wait_for_completion_timeout is unsigned long not > > int. This patch fixes up the declarations only. > > > > Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y > > CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m > > Why didn't you set the text above as the patch description (which is > empty at the moment)? > basically because the one-line is sufficient to understand the patch and the rest of the information is not relevant for the git log but only for the review if you think it is necessary to understand the patch I'll move it and resubmit. thanks for your review ! hofrat ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3 v2] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type
On Thu, Jan 29, 2015 at 10:38:39AM +0100, Nicholas Mc Guire wrote: > On Mon, 26 Jan 2015, Tomi Valkeinen wrote: > > > Hi, > > > > On 25/01/15 16:47, Nicholas Mc Guire wrote: > > > Signed-off-by: Nicholas Mc Guire > > > --- > > > > > > v2: fixed subject line > > > > > > The return type of wait_for_completion_timeout is unsigned long not > > > int. This patch fixes up the declarations only. > > > This line is relevant for the patch description. > > > Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y > > > CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m This line is not relevant. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/3] hv: hv_util: move vmbus_open() to a later place
Before the line vmbus_open() returns, srv->util_cb can be already running and the variablies, like util_fw_version, are needed by the srv->util_cb. So we have to make sure the variables are initialized before the vmbus_open(). CC: "K. Y. Srinivasan" Signed-off-by: Dexuan Cui --- drivers/hv/hv_util.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index 3b9c9ef..c5be773 100644 --- a/drivers/hv/hv_util.c +++ b/drivers/hv/hv_util.c @@ -340,12 +340,8 @@ static int util_probe(struct hv_device *dev, set_channel_read_state(dev->channel, false); - ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0, - srv->util_cb, dev->channel); - if (ret) - goto error; - hv_set_drvdata(dev, srv); + /* * Based on the host; initialize the framework and * service version numbers we will negotiate. @@ -365,6 +361,11 @@ static int util_probe(struct hv_device *dev, hb_srv_version = HB_VERSION; } + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0, + srv->util_cb, dev->channel); + if (ret) + goto error; + return 0; error: -- 1.9.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/3] hv: vmbus_post_msg: retry the hypercall on HV_STATUS_INVALID_CONNECTION_ID
I got the hypercall error code on Hyper-V 2008 R2 when keeping running
"rmmod hv_netvsc; modprobe hv_netvsc; rmmod hv_utils; modprobe hv_utils"
in a Linux guest.
Without the patch, the driver can occasionally fail to load.
CC: "K. Y. Srinivasan"
Signed-off-by: Dexuan Cui
---
arch/x86/include/uapi/asm/hyperv.h | 1 +
drivers/hv/connection.c| 9 +
2 files changed, 10 insertions(+)
diff --git a/arch/x86/include/uapi/asm/hyperv.h
b/arch/x86/include/uapi/asm/hyperv.h
index 90c458e..b9daffb 100644
--- a/arch/x86/include/uapi/asm/hyperv.h
+++ b/arch/x86/include/uapi/asm/hyperv.h
@@ -225,6 +225,7 @@
#define HV_STATUS_INVALID_HYPERCALL_CODE 2
#define HV_STATUS_INVALID_HYPERCALL_INPUT 3
#define HV_STATUS_INVALID_ALIGNMENT4
+#define HV_STATUS_INVALID_CONNECTION_ID18
#define HV_STATUS_INSUFFICIENT_BUFFERS 19
typedef struct _HV_REFERENCE_TSC_PAGE {
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index c4acd1c..8bd05f3 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -440,6 +440,15 @@ int vmbus_post_msg(void *buffer, size_t buflen)
ret = hv_post_message(conn_id, 1, buffer, buflen);
switch (ret) {
+ case HV_STATUS_INVALID_CONNECTION_ID:
+ /*
+* We could get this if we send messages too
+* frequently or the host is under low resource
+* conditions: let's wait 1 more second before
+* retrying the hypercall.
+*/
+ msleep(1000);
+ break;
case HV_STATUS_INSUFFICIENT_BUFFERS:
ret = -ENOMEM;
case -ENOMEM:
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
Without this patch, the state is put to CHANNEL_OPENING_STATE, and when
the driver is loaded next time, vmbus_open() will fail immediately due to
newchannel->state != CHANNEL_OPEN_STATE.
CC: "K. Y. Srinivasan"
Signed-off-by: Dexuan Cui
---
drivers/hv/channel.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 2978f5e..26dcf26 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel, u32
send_ringbuffer_size,
out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
get_order(send_ringbuffer_size + recv_ringbuffer_size));
- if (!out)
- return -ENOMEM;
-
+ if (!out) {
+ err = -ENOMEM;
+ goto error0;
+ }
in = (void *)((unsigned long)out + send_ringbuffer_size);
@@ -199,6 +200,7 @@ error0:
free_pages((unsigned long)out,
get_order(send_ringbuffer_size + recv_ringbuffer_size));
kfree(open_info);
+ newchannel->state = CHANNEL_OPEN_STATE;
return err;
}
EXPORT_SYMBOL_GPL(vmbus_open);
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] staging/nvec: reimplement on top of tegra i2c driver
Am Donnerstag, 29. Januar 2015, 10:20:21 schrieb Andrey Danin:
> Remove i2c controller related code and use tegra i2c driver in slave mode.
the diff is hard to review. Maybe it would be better to first ifdef 0 the old
code (isr and init) while adding the new code, and then remove the old code in
a second patch. Or maybe split it into small chunks.
> Signed-off-by: Andrey Danin
> ---
> drivers/staging/nvec/nvec.c | 379
> ++-- drivers/staging/nvec/nvec.h |
> 17 +-
> 2 files changed, 122 insertions(+), 274 deletions(-)
>
> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> index 120b70d..d645c58 100644
> --- a/drivers/staging/nvec/nvec.c
> +++ b/drivers/staging/nvec/nvec.c
> @@ -25,8 +25,8 @@
> #include
> #include
> #include
> +#include
> #include
> -#include
> #include
> #include
> #include
> @@ -39,25 +39,12 @@
>
> #include "nvec.h"
>
> -#define I2C_CNFG 0x00
> -#define I2C_CNFG_PACKET_MODE_EN (1<<10)
> -#define I2C_CNFG_NEW_MASTER_SFM (1<<11)
> -#define I2C_CNFG_DEBOUNCE_CNT_SHIFT 12
> -
> -#define I2C_SL_CNFG 0x20
> -#define I2C_SL_NEWSL (1<<2)
> -#define I2C_SL_NACK (1<<1)
> -#define I2C_SL_RESP (1<<0)
> -#define I2C_SL_IRQ (1<<3)
> -#define END_TRANS(1<<4)
> -#define RCVD (1<<2)
> -#define RNW (1<<1)
> -
> -#define I2C_SL_RCVD 0x24
> -#define I2C_SL_STATUS0x28
> -#define I2C_SL_ADDR1 0x2c
> -#define I2C_SL_ADDR2 0x30
> -#define I2C_SL_DELAY_COUNT 0x3c
> +
> +#define I2C_SL_ST_END_TRANS (1<<4)
> +#define I2C_SL_ST_IRQ(1<<3)
> +#define I2C_SL_ST_RCVD (1<<2)
> +#define I2C_SL_ST_RNW(1<<1)
> +
>
> /**
> * enum nvec_msg_category - Message categories for nvec_msg_alloc()
> @@ -327,6 +314,7 @@ struct nvec_msg *nvec_write_sync(struct nvec_chip *nvec,
>
> mutex_unlock(&nvec->sync_write_mutex);
>
> +
stray new line.
> return msg;
> }
> EXPORT_SYMBOL(nvec_write_sync);
> @@ -475,11 +463,13 @@ static void nvec_tx_completed(struct nvec_chip *nvec)
> {
> /* We got an END_TRANS, let's skip this, maybe there's an event */
> if (nvec->tx->pos != nvec->tx->size) {
> - dev_err(nvec->dev, "premature END_TRANS, resending\n");
> + dev_err(nvec->dev, "premature END_TRANS, resending: pos:%u,
> size:
%u\n",
> + nvec->tx->pos, nvec->tx->size);
> nvec->tx->pos = 0;
> nvec_gpio_set_value(nvec, 0);
> } else {
> - nvec->state = 0;
> + nvec->state = ST_NONE;
> + nvec->tx->pos = 0;
> }
> }
>
> @@ -497,7 +487,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
> (uint) nvec->rx->pos);
>
> nvec_msg_free(nvec, nvec->rx);
> - nvec->state = 0;
> + nvec->state = ST_NONE;
>
> /* Battery quirk - Often incomplete, and likes to crash */
> if (nvec->rx->data[0] == NVEC_BAT)
> @@ -514,7 +504,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
>
> spin_unlock(&nvec->rx_lock);
>
> - nvec->state = 0;
> + nvec->state = ST_NONE;
>
> if (!nvec_msg_is_event(nvec->rx))
> complete(&nvec->ec_transfer);
> @@ -523,21 +513,6 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
> }
>
> /**
> - * nvec_invalid_flags - Send an error message about invalid flags and jump
> - * @nvec: The nvec device
> - * @status: The status flags
> - * @reset: Whether we shall jump to state 0.
> - */
> -static void nvec_invalid_flags(struct nvec_chip *nvec, unsigned int status,
> -bool reset)
> -{
> - dev_err(nvec->dev, "unexpected status flags 0x%02x during state %i\n",
> - status, nvec->state);
> - if (reset)
> - nvec->state = 0;
> -}
> -
> -/**
> * nvec_tx_set - Set the message to transfer (nvec->tx)
> * @nvec: A &struct nvec_chip
> *
> @@ -566,150 +541,85 @@ static void nvec_tx_set(struct nvec_chip *nvec)
> (uint)nvec->tx->size, nvec->tx->data[1]);
> }
>
> +
> /**
> - * nvec_interrupt - Interrupt handler
> - * @irq: The IRQ
> - * @dev: The nvec device
> + * nvec_slave_cb - I2C slave callback
> *
> - * Interrupt handler that fills our RX buffers and empties our TX
> - * buffers. This uses a finite state machine with ridiculous amounts
> - * of error checking, in order to be fairly reliable.
> + * This callback fills our RX buffers and empties our TX
> + * buffers. This uses a finite state machine.
> */
> -static irqreturn_t nvec_interrupt(int irq, void *dev)
> +static int nvec_slave_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> {
> - unsigned long status;
> - un
Re: [PATCH] staging/fwserial: use correct vendor/version IDs
On Jan 28 Clemens Ladisch wrote:
> The driver was using the vendor ID 0xd00d1e from the FireWire core.
> However, this ID was not registered, and invalid.
>
> Instead, use the vendor/version IDs that now are officially assigned to
> firewire-serial:
> https://ieee1394.wiki.kernel.org/index.php/IEEE_OUI_Assignments
>
> Signed-off-by: Clemens Ladisch
> ---
[...]
> --- a/drivers/staging/fwserial/fwserial.c
> +++ b/drivers/staging/fwserial/fwserial.c
> @@ -30,8 +30,8 @@
>
> #define be32_to_u64(hi, lo) ((u64)be32_to_cpu(hi) << 32 | be32_to_cpu(lo))
>
> -#define LINUX_VENDOR_ID 0xd00d1eU /* same id used in card root directory
> */
> -#define FWSERIAL_VERSION 0x00e81cU /* must be unique within
> LINUX_VENDOR_ID */
> +#define LINUX_VENDOR_ID 0x001f11U /* same id used in card root directory
> */
> +#define FWSERIAL_VERSION 0x023953U /* must be unique within
> LINUX_VENDOR_ID */
(Bikeshedding: Per IEEE Registration Authority's list, 001f11 is not a
"Linux vendor ID" but Openmoko, Inc.'s vendor ID.)
Greg,
just FYI, the comment "same id used in card root directory" will remain
true only once a corresponding change to firewire-core was merged.
Proposed patch: http://marc.info/?l=linux1394-devel&m=142247554618207
(If I receive no substantial objections, this will go in in the next
merge window.)
Date: Wed, 28 Jan 2015 21:04:48 +0100
From: Clemens Ladisch
To: Stefan Richter
CC: linux1394-de...@lists.sourceforge.net
Subject: [PATCH] firewire: core: use correct vendor/model IDs
The kernel was using the vendor ID 0xd00d1e, which was inherited from
the old ieee1394 driver stack. However, this ID was not registered, and
invalid.
Instead, use the vendor/model IDs that are now officially assigned to
the kernel:
https://ieee1394.wiki.kernel.org/index.php/IEEE_OUI_Assignments
Signed-off-by: Clemens Ladisch
---
drivers/firewire/core-transaction.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firewire/core-transaction.c
b/drivers/firewire/core-transaction.c
index 60f6963..99d9d8c7 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -1271,14 +1271,14 @@ static u32 model_textual_descriptor[3 + 64/4] = {
static struct fw_descriptor vendor_id_descriptor = {
.length = ARRAY_SIZE(vendor_textual_descriptor),
- .immediate = 0x03d00d1e,
+ .immediate = 0x03001f11,
.key = 0x8100,
.data = vendor_textual_descriptor,
};
static struct fw_descriptor model_id_descriptor = {
.length = ARRAY_SIZE(model_textual_descriptor),
- .immediate = 0x1701,
+ .immediate = 0x17023901,
.key = 0x8100,
.data = model_textual_descriptor,
};
--
Stefan Richter
-=-= ---= ===-=
http://arcgraph.de/sr/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer
On Wed, Jan 28, 2015 at 7:57 PM, Dexuan Cui wrote:
-Original Message-
From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
Sent: Tuesday, January 20, 2015 23:45 PM
To: KY Srinivasan; de...@linuxdriverproject.org
Cc: Haiyang Zhang; linux-ker...@vger.kernel.org; Dexuan Cui; Jason
Wang;
Radim Krčmář; Dan Carpenter
Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and
Rescind offer
Commit 4b2f9abea52a ("staging: hv: convert channel_mgmt.c to not
call
osd_schedule_callback")' was written under an assumption that we
never
receive
Rescind offer while we're still processing the initial Offer
request. However,
the issue we fixed in 04a258c162a8 could be caused by this
assumption not
always being true.
In particular, we need to protect against the following:
1) Receiving a Rescind offer after we do queue_work() for
processing an
Offer
request and before we actually enter vmbus_process_offer().
work.func
points
to vmbus_process_offer() at this moment and in
vmbus_onoffer_rescind()
we do
another queue_work() without a check so we'll enter
vmbus_process_offer()
twice.
2) Receiving a Rescind offer after we enter vmbus_process_offer()
and
especially after we set >state = CHANNEL_OPEN_STATE. Many things
can go
wrong in that case, e.g. we can call free_channel() while we're
still using
it.
Implement the required protection by changing work->func at the
very end
of
vmbus_process_offer() and checking work->func in
vmbus_onoffer_rescind().
In
case we receive rescind offer during or before
vmbus_process_offer() is
done
we set rescind flag to true and we check it at the end of
vmbus_process_offer()
so such offer will not get lost.
Suggested-by: Radim Krčmář
Signed-off-by: Vitaly Kuznetsov
---
drivers/hv/channel_mgmt.c | 30 ++
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index c6fdd74..877a944 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -279,9 +279,6 @@ static void vmbus_process_offer(struct
work_struct
*work)
int ret;
unsigned long flags;
- /* The next possible work is rescind handling */
- INIT_WORK(&newchannel->work, vmbus_process_rescind_offer);
-
/* Make sure this is a new offer */
spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
@@ -341,7 +338,7 @@ static void vmbus_process_offer(struct
work_struct
*work)
if (channel->sc_creation_callback != NULL)
channel->sc_creation_callback(newchannel);
- goto out;
+ goto done_init_rescind;
}
goto err_free_chan;
@@ -382,7 +379,14 @@ static void vmbus_process_offer(struct
work_struct
*work)
kfree(newchannel->device_obj);
goto err_free_chan;
}
-out:
+done_init_rescind:
+ spin_lock_irqsave(&newchannel->lock, flags);
+ /* The next possible work is rescind handling */
+ INIT_WORK(&newchannel->work, vmbus_process_rescind_offer);
+ /* Check if rescind offer was already received */
+ if (newchannel->rescind)
+ queue_work(newchannel->controlwq, &newchannel->work);
+ spin_unlock_irqrestore(&newchannel->lock, flags);
return;
err_free_chan:
free_channel(newchannel);
@@ -520,6 +524,7 @@ static void vmbus_onoffer_rescind(struct
vmbus_channel_message_header *hdr)
{
struct vmbus_channel_rescind_offer *rescind;
struct vmbus_channel *channel;
+ unsigned long flags;
rescind = (struct vmbus_channel_rescind_offer *)hdr;
channel = relid2channel(rescind->child_relid);
@@ -528,11 +533,20 @@ static void vmbus_onoffer_rescind(struct
vmbus_channel_message_header *hdr)
/* Just return here, no channel found */
return;
+ spin_lock_irqsave(&channel->lock, flags);
channel->rescind = true;
+ /*
+ * channel->work.func != vmbus_process_rescind_offer means we
are still
+ * processing offer request and the rescind offer processing
should
be
+ * postponed. It will be done at the very end of
vmbus_process_offer()
+ * as rescind flag is being checked there.
+ */
+ if (channel->work.func == vmbus_process_rescind_offer)
+ /* work is initialized for vmbus_process_rescind_offer() from
+ * vmbus_process_offer() where the channel got created */
+ queue_work(channel->controlwq, &channel->work);
- /* work is initialized for vmbus_process_rescind_offer() from
- * vmbus_process_offer() where the channel got created */
- queue_work(channel->controlwq, &channel->work);
+ spin_unlock_irqrestore(&channel->lock, flags);
}
/*
--
Hi Vitaly and all,
I have 2 questions:
In vmbus_process_offer(), in the cases
RE: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer
On Wed, Jan 28, 2015 at 8:51 PM, Dexuan Cui wrote: -Original Message- From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] Sent: Wednesday, January 28, 2015 20:09 PM To: Dexuan Cui Cc: KY Srinivasan; de...@linuxdriverproject.org; Haiyang Zhang; linux- ker...@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer Dexuan Cui writes: >> -Original Message- >> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] >> Sent: Tuesday, January 20, 2015 23:45 PM >> To: KY Srinivasan; de...@linuxdriverproject.org >> Cc: Haiyang Zhang; linux-ker...@vger.kernel.org; Dexuan Cui; Jason Wang; >> Radim Krčmář; Dan Carpenter >> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer ... > > Hi Vitaly and all, > I have 2 questions: > In vmbus_process_offer(), in the cases of "goto err_free_chan", > should we consider the possibility a rescind message could be pending for > the new channel? > In the cases, because we don't run > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ", > vmbus_onoffer_rescind() will do nothing and as a result, > vmbus_process_rescind_offer() won't be invoked. Yes, but processing the rescind offer results in freeing the channel (and this processing supposes the channel wasn't freed before) so there is no difference... or is it? > > Question 2: in vmbus_process_offer(), in the case > vmbus_device_register() fails, we'll run > "list_del(&newchannel->listentry);" -- just after this line, > what will happen at this time if relid2channel() returns NULL > in vmbus_onoffer_rescind()? > > I think we'll lose the rescind message. > Yes, but same logic applies - we already freed the channes so no rescind proccessing required. free_channel() and vmbus_process_rescind_offer() are different, because the latter does more work, e.g., sending the host a message CHANNELMSG_RELID_RELEASED. In the cases of "goto err_free_chan" + "a pending rescind message", the host may expect the message CHANNELMSG_RELID_RELEASED and could reoffer the channel once the message is received. It would be better if the VM doesn't lose the rescind message here. :-) It's interesting that rescind needs a ack from guest. But looks like the offer does not need it? Is there a spec for this for us for reference? Thanks If we still need to do something we need to add support for already freed channel to the rescind offer processing path. Thanks, -- Dexuan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer
On Wed, Jan 28, 2015 at 9:09 PM, Vitaly Kuznetsov wrote: Dexuan Cui writes: -Original Message- From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] Sent: Wednesday, January 28, 2015 20:09 PM To: Dexuan Cui Cc: KY Srinivasan; de...@linuxdriverproject.org; Haiyang Zhang; linux- ker...@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer Dexuan Cui writes: >> -Original Message- >> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] >> Sent: Tuesday, January 20, 2015 23:45 PM >> To: KY Srinivasan; de...@linuxdriverproject.org >> Cc: Haiyang Zhang; linux-ker...@vger.kernel.org; Dexuan Cui; Jason Wang; >> Radim Krčmář; Dan Carpenter >> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer ... > > Hi Vitaly and all, > I have 2 questions: > In vmbus_process_offer(), in the cases of "goto err_free_chan", > should we consider the possibility a rescind message could be pending for > the new channel? > In the cases, because we don't run > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ", > vmbus_onoffer_rescind() will do nothing and as a result, > vmbus_process_rescind_offer() won't be invoked. Yes, but processing the rescind offer results in freeing the channel (and this processing supposes the channel wasn't freed before) so there is no difference... or is it? > > Question 2: in vmbus_process_offer(), in the case > vmbus_device_register() fails, we'll run > "list_del(&newchannel->listentry);" -- just after this line, > what will happen at this time if relid2channel() returns NULL > in vmbus_onoffer_rescind()? > > I think we'll lose the rescind message. > Yes, but same logic applies - we already freed the channes so no rescind proccessing required. free_channel() and vmbus_process_rescind_offer() are different, because the latter does more work, e.g., sending the host a message CHANNELMSG_RELID_RELEASED. In the cases of "goto err_free_chan" + "a pending rescind message", the host may expect the message CHANNELMSG_RELID_RELEASED and could reoffer the channel once the message is received. It would be better if the VM doesn't lose the rescind message here. :-) Ah, I see, CHANNELMSG_RELID_RELEASED is expected from us in any case. I'll doing that in a separate patch is noone objects. All the evil come from the un-serialized processing of message. I wonder whether we can do all the processing in workqueue and then those were automatically serialized. Thanks for the review, If we still need to do something we need to add support for already freed channel to the rescind offer processing path. Thanks, -- Dexuan -- Vitaly -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3 v2] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type
On Thu, 29 Jan 2015, Dan Carpenter wrote: > On Thu, Jan 29, 2015 at 10:38:39AM +0100, Nicholas Mc Guire wrote: > > On Mon, 26 Jan 2015, Tomi Valkeinen wrote: > > > > > Hi, > > > > > > On 25/01/15 16:47, Nicholas Mc Guire wrote: > > > > Signed-off-by: Nicholas Mc Guire > > > > --- > > > > > > > > v2: fixed subject line > > > > > > > > The return type of wait_for_completion_timeout is unsigned long not > > > > int. This patch fixes up the declarations only. > > > > > > This line is relevant for the patch description. > > > > > Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y > > > > CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m > > This line is not relevant. > thanks - will resend it then - assumed that the one-line would do for this patch. thx! hofrat ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC] staging: media: davinci_vpfe: drop condition with no effect
On Mon, Jan 26, 2015 at 7:27 AM, Nicholas Mc Guire wrote:
> As the if and else branch body are identical the condition has no effect and
> can be dropped.
>
> Signed-off-by: Nicholas Mc Guire
Acked-by: Lad, Prabhakar
Regards,
--Prabhakar Lad
> ---
>
> As the if and the else branch of the inner conditional paths are the same
> the condition is without effect. Given the comments indicate that
> the else branch *should* be handling a specific case this may indicate
> a bug, in which case the below patch is *wrong*. This needs a review by
> someone that knows the specifics of this driver.
>
> If the inner if/else is a placeholder for planed updates then it should
> be commented so this is clear.
>
> Patch was only compile tested with davinci_all_defconfig + CONFIG_STAGING=y
> CONFIG_STAGING_MEDIA=y, CONFIG_MEDIA_SUPPORT=m,
> CONFIG_MEDIA_ANALOG_TV_SUPPORT=y, CONFIG_MEDIA_CAMERA_SUPPORT=y
> CONFIG_MEDIA_CONTROLLER=y, CONFIG_VIDEO_V4L2_SUBDEV_API=y
> CONFIG_VIDEO_DM365_VPFE=m
>
> Patch is against 3.0.19-rc5 -next-20150123
>
> drivers/staging/media/davinci_vpfe/dm365_resizer.c |9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> index 75e70e1..bf2cb7a 100644
> --- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> +++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> @@ -63,16 +63,11 @@ resizer_calculate_line_length(u32 pix, int width, int
> height,
> if (pix == MEDIA_BUS_FMT_UYVY8_2X8 ||
> pix == MEDIA_BUS_FMT_SGRBG12_1X12) {
> *line_len = width << 1;
> - } else if (pix == MEDIA_BUS_FMT_Y8_1X8 ||
> - pix == MEDIA_BUS_FMT_UV8_1X8) {
> - *line_len = width;
> - *line_len_c = width;
> - } else {
> - /* YUV 420 */
> - /* round width to upper 32 byte boundary */
> + } else {
> *line_len = width;
> *line_len_c = width;
> }
> +
> /* adjust the line len to be a multiple of 32 */
> *line_len += 31;
> *line_len &= ~0x1f;
> --
> 1.7.10.4
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/3 v3] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type
The return type of wait_for_completion_timeout is unsigned long not int. This patch fixes up the declarations only. Signed-off-by: Nicholas Mc Guire --- v2: fixed subject line v3: fixed patch description as recommended by Dan Carpenter Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m Patch is against 3.19.0-rc5 -next-20150123 drivers/video/fbdev/hyperv_fb.c |6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c index 4254336..807ee22 100644 --- a/drivers/video/fbdev/hyperv_fb.c +++ b/drivers/video/fbdev/hyperv_fb.c @@ -415,7 +415,8 @@ static int synthvid_negotiate_ver(struct hv_device *hdev, u32 ver) struct fb_info *info = hv_get_drvdata(hdev); struct hvfb_par *par = info->par; struct synthvid_msg *msg = (struct synthvid_msg *)par->init_buf; - int t, ret = 0; + int ret = 0; + unsigned long t; memset(msg, 0, sizeof(struct synthvid_msg)); msg->vid_hdr.type = SYNTHVID_VERSION_REQUEST; @@ -488,7 +489,8 @@ static int synthvid_send_config(struct hv_device *hdev) struct fb_info *info = hv_get_drvdata(hdev); struct hvfb_par *par = info->par; struct synthvid_msg *msg = (struct synthvid_msg *)par->init_buf; - int t, ret = 0; + int ret = 0; + unsigned long t; /* Send VRAM location */ memset(msg, 0, sizeof(struct synthvid_msg)); -- 1.7.10.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3 v2] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type
On 29/01/15 11:38, Nicholas Mc Guire wrote: > On Mon, 26 Jan 2015, Tomi Valkeinen wrote: > >> Hi, >> >> On 25/01/15 16:47, Nicholas Mc Guire wrote: >>> Signed-off-by: Nicholas Mc Guire >>> --- >>> >>> v2: fixed subject line >>> >>> The return type of wait_for_completion_timeout is unsigned long not >>> int. This patch fixes up the declarations only. >>> >>> Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y >>> CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m >> >> Why didn't you set the text above as the patch description (which is >> empty at the moment)? >> > basically because the one-line is sufficient to understand the patch You didn't have one line, you had no description. Patch subject is not patch description. In the minimal case, the description should have the same text as the subject, but usually it's better to have a bit more text in the description. > and the rest of the information is not relevant for the git log but only > for the review > > if you think it is necessary to understand the patch I'll move it and > resubmit. Well, a good description is not only about understanding the code in the patch. It may contain information like which platform/setup this issue happened on, are the any possible side effects, or whatever might be relevant for someone looking at the patch years later. Tomi signature.asc Description: OpenPGP digital signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3 v2] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type
On Thu, 29 Jan 2015, Tomi Valkeinen wrote: > On 29/01/15 11:38, Nicholas Mc Guire wrote: > > On Mon, 26 Jan 2015, Tomi Valkeinen wrote: > > > >> Hi, > >> > >> On 25/01/15 16:47, Nicholas Mc Guire wrote: > >>> Signed-off-by: Nicholas Mc Guire > >>> --- > >>> > >>> v2: fixed subject line > >>> > >>> The return type of wait_for_completion_timeout is unsigned long not > >>> int. This patch fixes up the declarations only. > >>> > >>> Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y > >>> CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m > >> > >> Why didn't you set the text above as the patch description (which is > >> empty at the moment)? > >> > > basically because the one-line is sufficient to understand the patch > > You didn't have one line, you had no description. Patch subject is not > patch description. In the minimal case, the description should have the > same text as the subject, but usually it's better to have a bit more > text in the description. ok - was not clear about this - got it. > > > and the rest of the information is not relevant for the git log but only > > for the review > > > > if you think it is necessary to understand the patch I'll move it and > > resubmit. > > Well, a good description is not only about understanding the code in the > patch. It may contain information like which platform/setup this issue > happened on, are the any possible side effects, or whatever might be > relevant for someone looking at the patch years later. > yup - but it seemed to me that the information on the build config and kernel version details would not really be relevant for this cleanup patch - so if I got your right the description line should have gone up and the config/kernel info stays below "---". Just resent it - hope this is correct now. thx! hofrat ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8712: remove useless printing line
On Wed, Jan 28, 2015 at 12:00:22PM +0200, Heba Aamer wrote: > This patch removes an unneeded call to printk. > Thanks. :) regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8712: fix Prefer ether_addr_copy() over memcpy()
On Wed, Jan 28, 2015 at 11:30:11PM +0200, Aya Mahfouz wrote: > On Wed, Jan 28, 2015 at 10:48:40AM -0600, Larry Finger wrote: > > On 01/28/2015 09:53 AM, Heba Aamer wrote: > > >This patch fixes the following checkpatch.pl warning: > > >Prefer ether_addr_copy() over memcpy() > > >if the Ethernet addresses are __aligned(2) > > > > > >I used the following coccinelle script: > > > > > >@@ > > >expression E1,E2;constant E3; > > >@@ > > > > > >- memcpy(E1, E2, E3) > > >+ ether_addr_copy(E1, E2) > > > > > > > > >pahole showed that the used structs are aligned to u16. > > > > I think you can stop here. The commit message is much too long for a 2-line > > patch. > > > > BTW, have you tested this patch? In particular, it needs to be tested on an > > architecture where alignment is important. Using x86 is not sufficient. The > > reason I ask is that there have been a lot of patches lately that change > > locking and alignment issues that are only build tested, and have never been > > tested with real hardware on any platform. > > > > One other thing, checkpatch only suggests that this change should be made. > > It is certainly not mandatory. As you have not indicated that it has been > > tested, > > > > NACK > > > > Larry > > > Hello Larry, > > Thank you for your patience. Heba has submitted this patch as part > of a workshop she currently attends. She has checked the alignment > through pahole and it showed that the variables of complex structs > are aligned. She has attached the output of pahole, so that the > community can verify her results and hence the lengthy output. > > She can also cross compile the kernel and verify the output for > other architectures using pahole. Kindly let us know if this suits > you. And please name any specific architecture that you would to see > tested. If this is still not enough from your point of view, let > us know what should be done further to verify the correctness of > the patch. > Really, I hate this checkpatch.pl warning, too. The patches are difficult to review because you need a lot of context and there is a small chance that the patch will introduce a bug. I was the person who introduced the rule that the patch submitter has to prove the alignment is correct after two people told me basically that, "The patch submitter's job is to sed the code and the maintainer's job is to review the code." In this case we don't really need to use pahole. "mac" is a 6 byte char array declared on the stack after a couple of integers. pnetdev->dev_addr is a pointer. &pdata[0x12] is a pointer plus an even offset. This patch is fine. But the changelog is too long and has a lot of not at all relevant output from pahole. It's not really a practical thing to say that the patch writer has to cross compile on a different arch. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 02/02] STAGING: Fix pcl818.c coding style issue: line over 80 characters
On 29/01/15 05:34, Simon Guo wrote: Correct one coding style problem(detected by checkpatch.pl) in pcl818.c. - line over 80 characters Signed-off-by: Simon Guo --- drivers/staging/comedi/drivers/pcl818.c | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) Reviewed-by: Ian Abbott -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Web: http://www.mev.co.uk/ )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 01/02] STAGING: Fix pcl818.c coding style issue: code indent should use tabs where possible
On 29/01/15 05:33, Simon Guo wrote: Correct one coding style problem(detected by checkpatch.pl) in pcl818.c. - code indent should use tabs where possible It is fixed by reformatting the comment block to usual comment style. And with the reformatting, following coding style problem is also fixed: - please, no space before tabs Signed-off-by: Simon Guo --- drivers/staging/comedi/drivers/pcl818.c | 189 +++- 1 file changed, 91 insertions(+), 98 deletions(-) Reviewed-by: Ian Abbott -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Web: http://www.mev.co.uk/ )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: addi_apci_1500: fix array access out of bounds error
On 28/01/15 16:58, H Hartley Sweeten wrote: The private data 'pm', 'pt', and 'pp' array members hold the trigger mode parameters for ports A and B. Both ports are 8-bits and the arrays are 16-bits. Array index 0 defines the AND mode and index 1 the OR mode parameters for both ports. The valid triggers to start the async command are 0 to 3 which select the AND/OR mode for each port. The 'pb_trig' (the array index for port B) in apci1500_di_inttrig_start() is incorrect and results in an index of 0 or 2. Fix the calc so that the correct index (0/1) is used. Signed-off-by: H Hartley Sweeten Reported-by: Asaf Vertz Cc: Ian Abbott Cc: Greg Kroah-Hartman Reviewed-by: Ian Abbott -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Web: http://www.mev.co.uk/ )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: dt2814: Removed variables that is never used
On 28/01/15 22:33, Rickard Strandqvist wrote: Variable ar assigned a value that is never used. I have also removed all the code that thereby serves no purpose. This was found using a static code analysis program called cppcheck Signed-off-by: Rickard Strandqvist --- drivers/staging/comedi/drivers/dt2814.c | 13 - 1 file changed, 4 insertions(+), 9 deletions(-) Good, apart from the bad English! Reviewed-by: Ian Abbott -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Web: http://www.mev.co.uk/ )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: dt2814: Removed variables that is never used
On 29/01/15 12:52, Ian Abbott wrote: On 28/01/15 22:33, Rickard Strandqvist wrote: Variable ar assigned a value that is never used. I have also removed all the code that thereby serves no purpose. This was found using a static code analysis program called cppcheck Signed-off-by: Rickard Strandqvist --- drivers/staging/comedi/drivers/dt2814.c | 13 - 1 file changed, 4 insertions(+), 9 deletions(-) Good, apart from the bad English! Reviewed-by: Ian Abbott On second thoughts, I retract my 'Reviewed-by'. The patch actually serves to point out a glaring bug in the original code, in that it acquires data in the interrupt routine but just throws it all away! -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Web: http://www.mev.co.uk/ )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3 v3] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type
Nicholas Mc Guire writes: > The return type of wait_for_completion_timeout is unsigned long not > int. This patch fixes up the declarations only. > > Signed-off-by: Nicholas Mc Guire I would be slightly better to remove ".c" from your subject like, anyway: Reviewed-by: Vitaly Kuznetsov > --- > > v2: fixed subject line > v3: fixed patch description as recommended by Dan Carpenter > > > Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y > CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m > > Patch is against 3.19.0-rc5 -next-20150123 > > drivers/video/fbdev/hyperv_fb.c |6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c > index 4254336..807ee22 100644 > --- a/drivers/video/fbdev/hyperv_fb.c > +++ b/drivers/video/fbdev/hyperv_fb.c > @@ -415,7 +415,8 @@ static int synthvid_negotiate_ver(struct hv_device *hdev, > u32 ver) > struct fb_info *info = hv_get_drvdata(hdev); > struct hvfb_par *par = info->par; > struct synthvid_msg *msg = (struct synthvid_msg *)par->init_buf; > - int t, ret = 0; > + int ret = 0; > + unsigned long t; > > memset(msg, 0, sizeof(struct synthvid_msg)); > msg->vid_hdr.type = SYNTHVID_VERSION_REQUEST; > @@ -488,7 +489,8 @@ static int synthvid_send_config(struct hv_device *hdev) > struct fb_info *info = hv_get_drvdata(hdev); > struct hvfb_par *par = info->par; > struct synthvid_msg *msg = (struct synthvid_msg *)par->init_buf; > - int t, ret = 0; > + int ret = 0; > + unsigned long t; > > /* Send VRAM location */ > memset(msg, 0, sizeof(struct synthvid_msg)); -- Vitaly ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] hv: hv_util: move vmbus_open() to a later place
Dexuan Cui writes: > Before the line vmbus_open() returns, srv->util_cb can be already running > and the variablies, like util_fw_version, are needed by the srv->util_cb. > > So we have to make sure the variables are initialized before the vmbus_open(). > > CC: "K. Y. Srinivasan" > Signed-off-by: Dexuan Cui It is not said in the description but moving hv_set_drvdata() before vmbus_open() make sense in case probe and remove can collide (can they?). Reviewed-by: Vitaly Kuznetsov > --- > drivers/hv/hv_util.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c > index 3b9c9ef..c5be773 100644 > --- a/drivers/hv/hv_util.c > +++ b/drivers/hv/hv_util.c > @@ -340,12 +340,8 @@ static int util_probe(struct hv_device *dev, > > set_channel_read_state(dev->channel, false); > > - ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0, > - srv->util_cb, dev->channel); > - if (ret) > - goto error; > - > hv_set_drvdata(dev, srv); > + > /* >* Based on the host; initialize the framework and >* service version numbers we will negotiate. > @@ -365,6 +361,11 @@ static int util_probe(struct hv_device *dev, > hb_srv_version = HB_VERSION; > } > > + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0, > + srv->util_cb, dev->channel); > + if (ret) > + goto error; > + > return 0; > > error: -- Vitaly ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
Dexuan Cui writes:
> Without this patch, the state is put to CHANNEL_OPENING_STATE, and when
> the driver is loaded next time, vmbus_open() will fail immediately due to
> newchannel->state != CHANNEL_OPEN_STATE.
The patch makes sense, but I have one small doubt. We call vmbus_open
from probe functions of various devices. E.g. in hyperv-keyboard we
have:
error = vmbus_open(...)
if (error)
goto err_free_mem;
and we don't call vmbus_close(...) on this path so no
CHANNELMSG_CLOSECHANNEL will be send. Who's gonna retry probe? Wouldn't
it be better to close the channel?
>
> CC: "K. Y. Srinivasan"
> Signed-off-by: Dexuan Cui
> ---
> drivers/hv/channel.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 2978f5e..26dcf26 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel, u32
> send_ringbuffer_size,
> out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
> get_order(send_ringbuffer_size + recv_ringbuffer_size));
>
> - if (!out)
> - return -ENOMEM;
> -
> + if (!out) {
> + err = -ENOMEM;
> + goto error0;
> + }
>
> in = (void *)((unsigned long)out + send_ringbuffer_size);
>
> @@ -199,6 +200,7 @@ error0:
> free_pages((unsigned long)out,
> get_order(send_ringbuffer_size + recv_ringbuffer_size));
> kfree(open_info);
> + newchannel->state = CHANNEL_OPEN_STATE;
> return err;
> }
> EXPORT_SYMBOL_GPL(vmbus_open);
--
Vitaly
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] hv: vmbus_post_msg: retry the hypercall on HV_STATUS_INVALID_CONNECTION_ID
Dexuan Cui writes:
> I got the hypercall error code on Hyper-V 2008 R2 when keeping running
> "rmmod hv_netvsc; modprobe hv_netvsc; rmmod hv_utils; modprobe hv_utils"
> in a Linux guest.
>
> Without the patch, the driver can occasionally fail to load.
>
> CC: "K. Y. Srinivasan"
> Signed-off-by: Dexuan Cui
> ---
> arch/x86/include/uapi/asm/hyperv.h | 1 +
> drivers/hv/connection.c| 9 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/arch/x86/include/uapi/asm/hyperv.h
> b/arch/x86/include/uapi/asm/hyperv.h
> index 90c458e..b9daffb 100644
> --- a/arch/x86/include/uapi/asm/hyperv.h
> +++ b/arch/x86/include/uapi/asm/hyperv.h
> @@ -225,6 +225,7 @@
> #define HV_STATUS_INVALID_HYPERCALL_CODE 2
> #define HV_STATUS_INVALID_HYPERCALL_INPUT3
> #define HV_STATUS_INVALID_ALIGNMENT 4
> +#define HV_STATUS_INVALID_CONNECTION_ID 18
> #define HV_STATUS_INSUFFICIENT_BUFFERS 19
The gap beween 4 and 18 tells me there are other codes here ;-) Are they
all 'permanent failures'?
>
> typedef struct _HV_REFERENCE_TSC_PAGE {
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index c4acd1c..8bd05f3 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -440,6 +440,15 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> ret = hv_post_message(conn_id, 1, buffer, buflen);
>
> switch (ret) {
> + case HV_STATUS_INVALID_CONNECTION_ID:
> + /*
> + * We could get this if we send messages too
> + * frequently or the host is under low resource
> + * conditions: let's wait 1 more second before
> + * retrying the hypercall.
> + */
> + msleep(1000);
> + break;
In case it is our last try (No. 10) we will return '18' from the
function. I suggest we set ret = -ENOMEM here as well.
> case HV_STATUS_INSUFFICIENT_BUFFERS:
> ret = -ENOMEM;
Or should we treat these two equally? There is a smaller (100ms) sleep
between tries already, we can consider changing it instead.
> case -ENOMEM:
--
Vitaly
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: dt2815: Removed variables that is never used
On 28/01/15 22:34, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/dt2815.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt2815.c
b/drivers/staging/comedi/drivers/dt2815.c
index a98fb66..c5c0490 100644
--- a/drivers/staging/comedi/drivers/dt2815.c
+++ b/drivers/staging/comedi/drivers/dt2815.c
@@ -98,12 +98,11 @@ static int dt2815_ao_insn(struct comedi_device *dev, struct
comedi_subdevice *s,
struct dt2815_private *devpriv = dev->private;
int i;
int chan = CR_CHAN(insn->chanspec);
- unsigned int lo, hi;
+ unsigned int lo;
int ret;
for (i = 0; i < insn->n; i++) {
lo = ((data[i] & 0x0f) << 4) | (chan << 1) | 0x01;
- hi = (data[i] & 0xff0) >> 4;
ret = comedi_timeout(dev, s, insn, dt2815_ao_status, 0x00);
if (ret)
I think this highlights a bug in the driver. It ought to write the hi
byte to the DT2815_DATA register, but I can't find any register
programming manuals for this board (only a Windows driver manual - and
that's for Windows 3.1 and Windows 95).
My guess (based on kernel log messages that have since been removed from
the driver) is that the sequence should be:
1. wait for the DT2815_STATUS register to read 0x00
2. write the lo byte to the DT2815_DATA register
3. wait for the DT2815_STATUS register to read 0x10
4. write the hi byte to the DT2815_DATA register
and that step 4 is missing from the driver.
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Web: http://www.mev.co.uk/ )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: jr3_pci: Removed variables that is never used
On 28/01/15 22:35, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/jr3_pci.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/jr3_pci.c
b/drivers/staging/comedi/drivers/jr3_pci.c
index 81fab2d..5d4cca7 100644
--- a/drivers/staging/comedi/drivers/jr3_pci.c
+++ b/drivers/staging/comedi/drivers/jr3_pci.c
@@ -520,10 +520,9 @@ static struct jr3_pci_poll_delay
jr3_pci_poll_subdevice(struct comedi_subdevice
result = poll_delay_min_max(20, 100);
} else {
/* Set full scale */
- struct six_axis_t min_full_scale;
struct six_axis_t max_full_scale;
- min_full_scale = get_min_full_scales(channel);
+ get_min_full_scales(channel);
max_full_scale = get_max_full_scales(channel);
set_full_scales(channel, max_full_scale);
Yes, it doesn't appear to be needed. The driver used to have some
kernel logs that output the min and max full scale information, but it
didn't do anything else with min_full_scale.
The call to get_min_full_scales() and the function itself can also be
removed.
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Web: http://www.mev.co.uk/ )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: mite: Removed variables that is never used
On 28/01/15 22:36, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/mite.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/mite.c
b/drivers/staging/comedi/drivers/mite.c
index ffc9e61..8d2903b 100644
--- a/drivers/staging/comedi/drivers/mite.c
+++ b/drivers/staging/comedi/drivers/mite.c
@@ -494,9 +494,8 @@ EXPORT_SYMBOL_GPL(mite_bytes_read_from_memory_ub);
unsigned mite_dma_tcr(struct mite_channel *mite_chan)
{
struct mite_struct *mite = mite_chan->mite;
- int lkar;
- lkar = readl(mite->mite_io_addr + MITE_LKAR(mite_chan->channel));
+ readl(mite->mite_io_addr + MITE_LKAR(mite_chan->channel));
return readl(mite->mite_io_addr + MITE_TCR(mite_chan->channel));
}
EXPORT_SYMBOL_GPL(mite_dma_tcr);
Reading the MITE_LKAR(channel) register won't have any side-effects, so
that call to readl() can be removed altogether. In previous versions of
the driver, the value of lkar was only used in debugging messages.
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Web: http://www.mev.co.uk/ )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: ni_atmio: Removed variables that is never used
On 28/01/15 22:37, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/ni_atmio.c |2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_atmio.c
b/drivers/staging/comedi/drivers/ni_atmio.c
index 0c5ff28..301f154 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -300,7 +300,6 @@ static int ni_atmio_attach(struct comedi_device *dev,
struct comedi_devconfig *it)
{
const struct ni_board_struct *boardtype;
- struct ni_private *devpriv;
struct pnp_dev *isapnp_dev;
int ret;
unsigned long iobase;
@@ -310,7 +309,6 @@ static int ni_atmio_attach(struct comedi_device *dev,
ret = ni_alloc_private(dev);
if (ret)
return ret;
- devpriv = dev->private;
iobase = it->options[0];
irq = it->options[1];
Yes, devpriv is no longer needed here.
Reviewed-by: Ian Abbott
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Web: http://www.mev.co.uk/ )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: ni_mio_cs: Removed variables that is never used
On 28/01/15 22:37, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/ni_mio_cs.c |3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_mio_cs.c
b/drivers/staging/comedi/drivers/ni_mio_cs.c
index 9b201e4..b152330 100644
--- a/drivers/staging/comedi/drivers/ni_mio_cs.c
+++ b/drivers/staging/comedi/drivers/ni_mio_cs.c
@@ -163,7 +163,6 @@ static int mio_cs_auto_attach(struct comedi_device *dev,
{
struct pcmcia_device *link = comedi_to_pcmcia_dev(dev);
static const struct ni_board_struct *board;
- struct ni_private *devpriv;
int ret;
board = ni_getboardtype(dev, link);
@@ -188,8 +187,6 @@ static int mio_cs_auto_attach(struct comedi_device *dev,
if (ret)
return ret;
- devpriv = dev->private;
-
return ni_E_init(dev, 0, 1);
}
Yes, devpriv is not needed here.
Reviewed-by: Ian Abbott
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Web: http://www.mev.co.uk/ )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: addi-data: hwdrv_apci3501: Removed variables that is never used
On 28/01/15 21:51, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c
b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c
index 339519a..24126e3 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c
@@ -93,7 +93,6 @@ static int apci3501_write_insn_timer(struct comedi_device
*dev,
{
struct apci3501_private *devpriv = dev->private;
unsigned int ul_Command1 = 0;
- int i_Temp;
if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
@@ -135,7 +134,7 @@ static int apci3501_write_insn_timer(struct comedi_device
*dev,
}
}
- i_Temp = inl(dev->iobase + APCI3501_TIMER_STATUS_REG) & 0x1;
+ inl(dev->iobase + APCI3501_TIMER_STATUS_REG) & 0x1;
The ' & 0x1' part produces compiler warnings and should be removed.
I don't know if the inl() call has any side-effects or not, but since
it's part of the watchdog functionality of this board, it's safest to
leave it in.
return insn->n;
}
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Web: http://www.mev.co.uk/ )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: addi_apci_3501: Removed variables that is never used
On 28/01/15 22:33, Rickard Strandqvist wrote: Variable ar assigned a value that is never used. I have also removed all the code that thereby serves no purpose. This was found using a static code analysis program called cppcheck Signed-off-by: Rickard Strandqvist --- drivers/staging/comedi/drivers/addi_apci_3501.c |3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/comedi/drivers/addi_apci_3501.c b/drivers/staging/comedi/drivers/addi_apci_3501.c index a726efc..0cdcecc 100644 --- a/drivers/staging/comedi/drivers/addi_apci_3501.c +++ b/drivers/staging/comedi/drivers/addi_apci_3501.c @@ -267,7 +267,6 @@ static irqreturn_t apci3501_interrupt(int irq, void *d) struct apci3501_private *devpriv = dev->private; unsigned int ui_Timer_AOWatchdog; unsigned long ul_Command1; - int i_temp; /* Disable Interrupt */ ul_Command1 = inl(dev->iobase + APCI3501_TIMER_CTRL_REG); @@ -285,7 +284,7 @@ static irqreturn_t apci3501_interrupt(int irq, void *d) ul_Command1 = inl(dev->iobase + APCI3501_TIMER_CTRL_REG); ul_Command1 = ((ul_Command1 & 0xF9FDul) | 1 << 1); outl(ul_Command1, dev->iobase + APCI3501_TIMER_CTRL_REG); - i_temp = inl(dev->iobase + APCI3501_TIMER_STATUS_REG) & 0x1; + inl(dev->iobase + APCI3501_TIMER_STATUS_REG) & 0x1; Same as for hwdrv_apci3501.c, the ' & 0x1' part should be removed to avoid compiler warnings. Keep the inl() call as it might have side-effects for the watchdog functionality of the board. return IRQ_HANDLED; } -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Web: http://www.mev.co.uk/ )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: dyna_pci10xx: Removed variables that is never used
On 28/01/15 22:35, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/dyna_pci10xx.c |5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dyna_pci10xx.c
b/drivers/staging/comedi/drivers/dyna_pci10xx.c
index 1b6324c..8882423 100644
--- a/drivers/staging/comedi/drivers/dyna_pci10xx.c
+++ b/drivers/staging/comedi/drivers/dyna_pci10xx.c
@@ -115,10 +115,9 @@ static int dyna_pci10xx_insn_write_ao(struct comedi_device
*dev,
{
struct dyna_pci10xx_private *devpriv = dev->private;
int n;
- unsigned int chan, range;
- chan = CR_CHAN(insn->chanspec);
- range = range_codes_pci1050_ai[CR_RANGE((insn->chanspec))];
+ CR_CHAN(insn->chanspec);
+ range_codes_pci1050_ai[CR_RANGE((insn->chanspec))];
Both those lines produce compiler warnings, are not needed, don't belong
here and should be removed. The AO subdevice has a single, fixed range.
mutex_lock(&devpriv->mutex);
for (n = 0; n < insn->n; n++) {
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Web: http://www.mev.co.uk/ )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: rtd520: Removed variables that is never used
On 28/01/15 22:38, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/rtd520.c |6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/comedi/drivers/rtd520.c
b/drivers/staging/comedi/drivers/rtd520.c
index 581aa58..305631c 100644
--- a/drivers/staging/comedi/drivers/rtd520.c
+++ b/drivers/staging/comedi/drivers/rtd520.c
@@ -1031,8 +1031,6 @@ static int rtd_ai_cmd(struct comedi_device *dev, struct
comedi_subdevice *s)
static int rtd_ai_cancel(struct comedi_device *dev, struct comedi_subdevice
*s)
{
struct rtd_private *devpriv = dev->private;
- u32 overrun;
- u16 status;
/* pacer stop source: SOFTWARE */
writel(0, dev->mmio + LAS0_PACER_STOP);
@@ -1040,8 +1038,8 @@ static int rtd_ai_cancel(struct comedi_device *dev,
struct comedi_subdevice *s)
writel(0, dev->mmio + LAS0_ADC_CONVERSION);
writew(0, dev->mmio + LAS0_IT);
devpriv->ai_count = 0; /* stop and don't transfer any more */
- status = readw(dev->mmio + LAS0_IT);
- overrun = readl(dev->mmio + LAS0_OVERRUN) & 0x;
+ readw(dev->mmio + LAS0_IT);
+ readl(dev->mmio + LAS0_OVERRUN) & 0x;
The ' & 0x' part produces compiler warnings.
Those two lines can be removed. The values read were previously only
used in a kernel log message.
writel(0, dev->mmio + LAS0_ADC_FIFO_CLEAR);
return 0;
}
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Web: http://www.mev.co.uk/ )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: usbduxsigma: Removed variables that is never used
On 28/01/15 22:39, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/usbduxsigma.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c
b/drivers/staging/comedi/drivers/usbduxsigma.c
index dc19435..7b80887 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -215,7 +215,6 @@ static void usbduxsigma_ai_handle_urb(struct comedi_device
*dev,
struct usbduxsigma_private *devpriv = dev->private;
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = &async->cmd;
- unsigned int dio_state;
uint32_t val;
int ret;
int i;
@@ -225,7 +224,7 @@ static void usbduxsigma_ai_handle_urb(struct comedi_device
*dev,
devpriv->ai_counter = devpriv->ai_timer;
/* get the state of the dio pins to allow external trigger */
- dio_state = be32_to_cpu(devpriv->in_buf[0]);
+ be32_to_cpu(devpriv->in_buf[0]);
That line produces a compiler warning. The line can be removed as the
call to be32_to_cpu() has no side-effects. The "/* get the state ..."
comment line can also be removed as it is only associated with this line.
Perhaps Bernd intended to use dio_state for something here, but I'm not
sure what.
/* get the data from the USB bus and hand it over to comedi */
for (i = 0; i < cmd->chanlist_len; i++) {
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Web: http://www.mev.co.uk/ )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: usbduxsigma: Removed variables that is never used
Indeed. It can be completely removed. I was intending to speed up DIO
reads during async acquisition but I decided against it because it would
create unpredictable latencies.
Thanks Ian for flagging it!
/Bernd
Ian Abbott wrote:
On 28/01/15 22:39, Rickard Strandqvist wrote:
Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/usbduxsigma.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c
b/drivers/staging/comedi/drivers/usbduxsigma.c
index dc19435..7b80887 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -215,7 +215,6 @@ static void usbduxsigma_ai_handle_urb(struct
comedi_device *dev,
struct usbduxsigma_private *devpriv = dev->private;
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = &async->cmd;
-unsigned int dio_state;
uint32_t val;
int ret;
int i;
@@ -225,7 +224,7 @@ static void usbduxsigma_ai_handle_urb(struct
comedi_device *dev,
devpriv->ai_counter = devpriv->ai_timer;
/* get the state of the dio pins to allow external trigger */
-dio_state = be32_to_cpu(devpriv->in_buf[0]);
+be32_to_cpu(devpriv->in_buf[0]);
That line produces a compiler warning. The line can be removed as the
call to be32_to_cpu() has no side-effects. The "/* get the state ..."
comment line can also be removed as it is only associated with this line.
Perhaps Bernd intended to use dio_state for something here, but I'm not
sure what.
/* get the data from the USB bus and hand it over to comedi */
for (i = 0; i < cmd->chanlist_len; i++) {
--
http://www.berndporr.me.uk
http://www.linux-usb-daq.co.uk
http://www.imdb.com/name/nm3293421/
+44 (0)7840 340069
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: gdm724x: gdm_tty: Fix for possible null pointer dereference
Fix a possible null pointer dereference, there is
otherwise a risk of a possible null pointer dereference.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/gdm724x/gdm_tty.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/gdm724x/gdm_tty.c
b/drivers/staging/gdm724x/gdm_tty.c
index 001348c..66b356e 100644
--- a/drivers/staging/gdm724x/gdm_tty.c
+++ b/drivers/staging/gdm724x/gdm_tty.c
@@ -145,7 +145,7 @@ static int gdm_tty_recv_complete(void *data,
struct gdm *gdm = tty_dev->gdm[index];
if (!GDM_TTY_READY(gdm)) {
- if (complete == RECV_PACKET_PROCESS_COMPLETE)
+ if (gdm && complete == RECV_PACKET_PROCESS_COMPLETE)
gdm_tty_recv(gdm, gdm_tty_recv_complete);
return TO_HOST_PORT_CLOSE;
}
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: include: lustre_update.h: Fix for possible null pointer dereference
Fix a possible null pointer dereference, there is otherwise a risk of a possible null pointer dereference. This was found using a static code analysis program called cppcheck Signed-off-by: Rickard Strandqvist --- drivers/staging/lustre/lustre/include/lustre_update.h |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/staging/lustre/lustre/include/lustre_update.h b/drivers/staging/lustre/lustre/include/lustre_update.h index 84defce..00e1361 100644 --- a/drivers/staging/lustre/lustre/include/lustre_update.h +++ b/drivers/staging/lustre/lustre/include/lustre_update.h @@ -165,12 +165,14 @@ static inline int update_get_reply_buf(struct update_reply *reply, void **buf, int result; ptr = update_get_buf_internal(reply, index, &size); + + LASSERT((ptr != NULL && size >= sizeof(int))); + result = *(int *)ptr; if (result < 0) return result; - LASSERT((ptr != NULL && size >= sizeof(int))); *buf = ptr + sizeof(int); return size - sizeof(int); } -- 1.7.10.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference
Fix a possible null pointer dereference, there is
otherwise a risk of a possible null pointer dereference.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/media/lirc/lirc_zilog.c |4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/media/lirc/lirc_zilog.c
b/drivers/staging/media/lirc/lirc_zilog.c
index cc872fb..78ce3b0 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1332,10 +1332,8 @@ static int close(struct inode *node, struct file *filep)
/* find our IR struct */
struct IR *ir = filep->private_data;
- if (ir == NULL) {
- dev_err(ir->l.dev, "close: no private_data attached to the
file!\n");
+ if (ir == NULL)
return -ENODEV;
- }
atomic_dec(&ir->open_count);
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8192u: r8192U_core: Fix for possible null pointer dereference
Fix a possible null pointer dereference, there is
otherwise a risk of a possible null pointer dereference.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/rtl8192u/r8192U_core.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/rtl8192u/r8192U_core.c
b/drivers/staging/rtl8192u/r8192U_core.c
index e031a25..922fd8b 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -963,6 +963,7 @@ static void rtl8192_hard_data_xmit(struct sk_buff *skb,
struct net_device *dev,
memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
tcb_desc->bTxEnableFwCalcDur = 1;
skb_push(skb, priv->ieee80211->tx_headroom);
+ // cppcheck-suppress unreadVariable CPPC_DONE
ret = rtl8192_tx(dev, skb);
spin_unlock_irqrestore(&priv->tx_lock, flags);
@@ -2530,6 +2531,7 @@ static short rtl8192_init(struct net_device *dev)
memset(priv->txqueue_to_outpipemap, 0, 9);
#ifdef PIPE12
{
+ // cppcheck-suppress unreadVariable CPPC_DONE
int i = 0;
u8 queuetopipe[] = {3, 2, 1, 0, 4, 8, 7, 6, 5};
memcpy(priv->txqueue_to_outpipemap, queuetopipe, 9);
@@ -3404,6 +3406,7 @@ void rtl8192_commit(struct net_device *dev)
ieee80211_softmac_stop_protocol(priv->ieee80211);
rtl8192_rtx_disable(dev);
+ // cppcheck-suppress unreadVariable CPPC_DONE
reset_status = _rtl8192_up(dev);
}
@@ -3721,6 +3724,7 @@ static void rtl8192_process_phyinfo(struct r8192_priv
*priv, u8 *buffer,
unsigned int frag, seq;
hdr = (struct ieee80211_hdr_3addr *)buffer;
sc = le16_to_cpu(hdr->seq_ctl);
+ // cppcheck-suppress unreadVariable CPPC_DONE
frag = WLAN_GET_SEQ_FRAG(sc);
seq = WLAN_GET_SEQ_SEQ(sc);
//cosa add 04292008 to record the sequence number
@@ -4476,11 +4480,11 @@ static void query_rxdesc_status(struct sk_buff *skb,
/* for debug 2008.5.29 */
- //added by vivi, for MP, 20080108
- stats->RxIs40MHzPacket = driver_info->BW;
- if (stats->RxDrvInfoSize != 0)
+ if (driver_info && stats->RxDrvInfoSize != 0) {
+ //added by vivi, for MP, 20080108
+ stats->RxIs40MHzPacket = driver_info->BW;
TranslateRxSignalStuff819xUsb(skb, stats, driver_info);
-
+ }
}
static void rtl8192_rx_nomal(struct sk_buff *skb)
@@ -4544,7 +4548,9 @@ static void rtl819xusb_process_received_packet(struct
net_device *dev,
// Get shifted bytes of Starting address of 802.11 header. 2006.09.28,
by Emily
//porting by amy 080508
pstats->virtual_address += get_rxpacket_shiftbytes_819xusb(pstats);
+ // cppcheck-suppress unreadVariable CPPC_TODO
frame = pstats->virtual_address;
+ // cppcheck-suppress unreadVariable CPPC_TODO
frame_len = pstats->packetlength;
#ifdef TODO// by amy about HCT
if (!Adapter->bInHctTest)
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: unisys: virtpci: virtpci: Fix for possible null pointer dereference
Fix a possible null pointer dereference, there is
otherwise a risk of a possible null pointer dereference.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/unisys/virtpci/virtpci.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/unisys/virtpci/virtpci.c
b/drivers/staging/unisys/virtpci/virtpci.c
index 39b828d..eea3c64 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -1340,16 +1340,16 @@ static ssize_t virtpci_driver_attr_show(struct kobject
*kobj,
struct driver_private *dprivate = to_driver(kobj);
struct device_driver *driver;
- if (dprivate != NULL)
+ if (dprivate != NULL) {
driver = dprivate->driver;
- else
- driver = NULL;
- DBGINF("In virtpci_driver_attr_show driver->name:%s\n", driver->name);
- if (driver) {
+ DBGINF("In virtpci_driver_attr_show driver->name:%s\n",
driver->name);
+
if (dattr->show)
ret = dattr->show(driver, buf);
}
+ else
+ DBGINF("In virtpci_driver_attr_show driver:NULL\n");
return ret;
}
@@ -1363,17 +1363,17 @@ static ssize_t virtpci_driver_attr_store(struct kobject
*kobj,
struct driver_private *dprivate = to_driver(kobj);
struct device_driver *driver;
- if (dprivate != NULL)
+ if (dprivate != NULL) {
driver = dprivate->driver;
- else
- driver = NULL;
- DBGINF("In virtpci_driver_attr_store driver->name:%s\n", driver->name);
+ DBGINF("In virtpci_driver_attr_store driver->name:%s\n",
driver->name);
- if (driver) {
if (dattr->store)
ret = dattr->store(driver, buf, count);
}
+ else
+ DBGINF("In virtpci_driver_attr_store driver:NULL\n");
+
return ret;
}
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192u: r8192U_core: Fix for possible null pointer dereference
2015-01-29 19:49 GMT+01:00 Rickard Strandqvist
:
> Fix a possible null pointer dereference, there is
> otherwise a risk of a possible null pointer dereference.
>
> This was found using a static code analysis program called cppcheck
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/staging/rtl8192u/r8192U_core.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c
> b/drivers/staging/rtl8192u/r8192U_core.c
> index e031a25..922fd8b 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -963,6 +963,7 @@ static void rtl8192_hard_data_xmit(struct sk_buff *skb,
> struct net_device *dev,
> memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
> tcb_desc->bTxEnableFwCalcDur = 1;
> skb_push(skb, priv->ieee80211->tx_headroom);
> + // cppcheck-suppress unreadVariable CPPC_DONE
> ret = rtl8192_tx(dev, skb);
>
> spin_unlock_irqrestore(&priv->tx_lock, flags);
> @@ -2530,6 +2531,7 @@ static short rtl8192_init(struct net_device *dev)
> memset(priv->txqueue_to_outpipemap, 0, 9);
> #ifdef PIPE12
> {
> + // cppcheck-suppress unreadVariable CPPC_DONE
> int i = 0;
> u8 queuetopipe[] = {3, 2, 1, 0, 4, 8, 7, 6, 5};
> memcpy(priv->txqueue_to_outpipemap, queuetopipe, 9);
> @@ -3404,6 +3406,7 @@ void rtl8192_commit(struct net_device *dev)
> ieee80211_softmac_stop_protocol(priv->ieee80211);
>
> rtl8192_rtx_disable(dev);
> + // cppcheck-suppress unreadVariable CPPC_DONE
> reset_status = _rtl8192_up(dev);
>
> }
> @@ -3721,6 +3724,7 @@ static void rtl8192_process_phyinfo(struct r8192_priv
> *priv, u8 *buffer,
> unsigned int frag, seq;
> hdr = (struct ieee80211_hdr_3addr *)buffer;
> sc = le16_to_cpu(hdr->seq_ctl);
> + // cppcheck-suppress unreadVariable CPPC_DONE
> frag = WLAN_GET_SEQ_FRAG(sc);
> seq = WLAN_GET_SEQ_SEQ(sc);
> //cosa add 04292008 to record the sequence number
> @@ -4476,11 +4480,11 @@ static void query_rxdesc_status(struct sk_buff *skb,
>
> /* for debug 2008.5.29 */
>
> - //added by vivi, for MP, 20080108
> - stats->RxIs40MHzPacket = driver_info->BW;
> - if (stats->RxDrvInfoSize != 0)
> + if (driver_info && stats->RxDrvInfoSize != 0) {
> + //added by vivi, for MP, 20080108
> + stats->RxIs40MHzPacket = driver_info->BW;
> TranslateRxSignalStuff819xUsb(skb, stats, driver_info);
> -
> + }
> }
>
> static void rtl8192_rx_nomal(struct sk_buff *skb)
> @@ -4544,7 +4548,9 @@ static void rtl819xusb_process_received_packet(struct
> net_device *dev,
> // Get shifted bytes of Starting address of 802.11 header.
> 2006.09.28, by Emily
> //porting by amy 080508
> pstats->virtual_address += get_rxpacket_shiftbytes_819xusb(pstats);
> + // cppcheck-suppress unreadVariable CPPC_TODO
> frame = pstats->virtual_address;
> + // cppcheck-suppress unreadVariable CPPC_TODO
> frame_len = pstats->packetlength;
> #ifdef TODO// by amy about HCT
> if (!Adapter->bInHctTest)
> --
> 1.7.10.4
>
Hi
Sorry, for the cppcheck-suppress part.
New patch on the way.
Kind regards
Rickard Strandqvist
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: rtl8192u: r8192U_core: Fix for possible null pointer dereference
Fix a possible null pointer dereference, there is
otherwise a risk of a possible null pointer dereference.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/rtl8192u/r8192U_core.c |8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/rtl8192u/r8192U_core.c
b/drivers/staging/rtl8192u/r8192U_core.c
index e031a25..4a29237 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -4476,11 +4476,11 @@ static void query_rxdesc_status(struct sk_buff *skb,
/* for debug 2008.5.29 */
- //added by vivi, for MP, 20080108
- stats->RxIs40MHzPacket = driver_info->BW;
- if (stats->RxDrvInfoSize != 0)
+ if (driver_info && stats->RxDrvInfoSize != 0) {
+ //added by vivi, for MP, 20080108
+ stats->RxIs40MHzPacket = driver_info->BW;
TranslateRxSignalStuff819xUsb(skb, stats, driver_info);
-
+ }
}
static void rtl8192_rx_nomal(struct sk_buff *skb)
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH net] hyperv: Fix the error processing in netvsc_send()
The existing code frees the skb in EAGAIN case, in which the skb will be
retried from upper layer and used again.
Also, the existing code doesn't free send buffer slot in error case, because
there is no completion message for unsent packets.
This patch fixes these problems.
(Please also include this patch for stable trees. Thanks!)
Signed-off-by: Haiyang Zhang
Reviewed-by: K. Y. Srinivasan
---
drivers/net/hyperv/netvsc.c | 11 ---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 9f49c01..7cd4eb3 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -716,7 +716,7 @@ int netvsc_send(struct hv_device *device,
u64 req_id;
unsigned int section_index = NETVSC_INVALID_INDEX;
u32 msg_size = 0;
- struct sk_buff *skb;
+ struct sk_buff *skb = NULL;
u16 q_idx = packet->q_idx;
@@ -743,8 +743,6 @@ int netvsc_send(struct hv_device *device,
packet);
skb = (struct sk_buff *)
(unsigned long)packet->send_completion_tid;
- if (skb)
- dev_kfree_skb_any(skb);
packet->page_buf_cnt = 0;
}
}
@@ -810,6 +808,13 @@ int netvsc_send(struct hv_device *device,
packet, ret);
}
+ if (ret != 0) {
+ if (section_index != NETVSC_INVALID_INDEX)
+ netvsc_free_send_slot(net_device, section_index);
+ } else if (skb) {
+ dev_kfree_skb_any(skb);
+ }
+
return ret;
}
--
1.7.4.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [HPDD-discuss] [PATCH] staging: lustre: include: lustre_update.h: Fix for possible null pointer dereference
On 01/29/2015 12:47 PM, Rickard Strandqvist wrote: Fix a possible null pointer dereference, there is otherwise a risk of a possible null pointer dereference. This was found using a static code analysis program called cppcheck Signed-off-by: Rickard Strandqvist --- drivers/staging/lustre/lustre/include/lustre_update.h |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/staging/lustre/lustre/include/lustre_update.h b/drivers/staging/lustre/lustre/include/lustre_update.h index 84defce..00e1361 100644 --- a/drivers/staging/lustre/lustre/include/lustre_update.h +++ b/drivers/staging/lustre/lustre/include/lustre_update.h @@ -165,12 +165,14 @@ static inline int update_get_reply_buf(struct update_reply *reply, void **buf, int result; ptr = update_get_buf_internal(reply, index, &size); + + LASSERT((ptr != NULL && size >= sizeof(int))); Now size is tested before result. So it could assert if result < 0, while the function would have returned before. + result = *(int *)ptr; if (result < 0) return result; - LASSERT((ptr != NULL && size >= sizeof(int))); *buf = ptr + sizeof(int); return size - sizeof(int); } ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [HPDD-discuss] [PATCH] staging: lustre: include: lustre_update.h: Fix for possible null pointer dereference
2015-01-29 20:40 GMT+01:00 Frank Zago : > On 01/29/2015 12:47 PM, Rickard Strandqvist wrote: >> >> Fix a possible null pointer dereference, there is >> otherwise a risk of a possible null pointer dereference. >> >> This was found using a static code analysis program called cppcheck >> >> Signed-off-by: Rickard Strandqvist >> >> --- >> drivers/staging/lustre/lustre/include/lustre_update.h |4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/staging/lustre/lustre/include/lustre_update.h >> b/drivers/staging/lustre/lustre/include/lustre_update.h >> index 84defce..00e1361 100644 >> --- a/drivers/staging/lustre/lustre/include/lustre_update.h >> +++ b/drivers/staging/lustre/lustre/include/lustre_update.h >> @@ -165,12 +165,14 @@ static inline int update_get_reply_buf(struct >> update_reply *reply, void **buf, >> int result; >> >> ptr = update_get_buf_internal(reply, index, &size); >> + >> + LASSERT((ptr != NULL && size >= sizeof(int))); > > > Now size is tested before result. So it could assert if result < 0, while > the function would have returned before. > > >> + >> result = *(int *)ptr; >> >> if (result < 0) >> return result; >> >> - LASSERT((ptr != NULL && size >= sizeof(int))); >> *buf = ptr + sizeof(int); >> return size - sizeof(int); >> } >> > But if prt is null krachar on the line: result = *(int *)ptr; Maybe there should be two LASSERT then. Kind regards Rickard Strandqvist ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [HPDD-discuss] [PATCH] staging: lustre: include: lustre_update.h: Fix for possible null pointer dereference
On 01/29/2015 01:47 PM, Rickard Strandqvist wrote: 2015-01-29 20:40 GMT+01:00 Frank Zago : On 01/29/2015 12:47 PM, Rickard Strandqvist wrote: Fix a possible null pointer dereference, there is otherwise a risk of a possible null pointer dereference. This was found using a static code analysis program called cppcheck Signed-off-by: Rickard Strandqvist --- drivers/staging/lustre/lustre/include/lustre_update.h |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/staging/lustre/lustre/include/lustre_update.h b/drivers/staging/lustre/lustre/include/lustre_update.h index 84defce..00e1361 100644 --- a/drivers/staging/lustre/lustre/include/lustre_update.h +++ b/drivers/staging/lustre/lustre/include/lustre_update.h @@ -165,12 +165,14 @@ static inline int update_get_reply_buf(struct update_reply *reply, void **buf, int result; ptr = update_get_buf_internal(reply, index, &size); + + LASSERT((ptr != NULL && size >= sizeof(int))); Now size is tested before result. So it could assert if result < 0, while the function would have returned before. + result = *(int *)ptr; if (result < 0) return result; - LASSERT((ptr != NULL && size >= sizeof(int))); *buf = ptr + sizeof(int); return size - sizeof(int); } But if prt is null krachar on the line: result = *(int *)ptr; Maybe there should be two LASSERT then. Yes, that would be safer. Frank. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8712: fix Prefer ether_addr_copy() over memcpy()
On Thu, Jan 29, 2015 at 02:51:57PM +0300, Dan Carpenter wrote: > On Wed, Jan 28, 2015 at 11:30:11PM +0200, Aya Mahfouz wrote: > > On Wed, Jan 28, 2015 at 10:48:40AM -0600, Larry Finger wrote: > > > On 01/28/2015 09:53 AM, Heba Aamer wrote: > > > >This patch fixes the following checkpatch.pl warning: > > > >Prefer ether_addr_copy() over memcpy() > > > >if the Ethernet addresses are __aligned(2) > > > > > > > >I used the following coccinelle script: > > > > > > > >@@ > > > >expression E1,E2;constant E3; > > > >@@ > > > > > > > >- memcpy(E1, E2, E3) > > > >+ ether_addr_copy(E1, E2) > > > > > > > > > > > >pahole showed that the used structs are aligned to u16. > > > > > > I think you can stop here. The commit message is much too long for a > > > 2-line patch. > > > > > > BTW, have you tested this patch? In particular, it needs to be tested on > > > an > > > architecture where alignment is important. Using x86 is not sufficient. > > > The > > > reason I ask is that there have been a lot of patches lately that change > > > locking and alignment issues that are only build tested, and have never > > > been > > > tested with real hardware on any platform. > > > > > > One other thing, checkpatch only suggests that this change should be made. > > > It is certainly not mandatory. As you have not indicated that it has been > > > tested, > > > > > > NACK > > > > > > Larry > > > > > Hello Larry, > > > > Thank you for your patience. Heba has submitted this patch as part > > of a workshop she currently attends. She has checked the alignment > > through pahole and it showed that the variables of complex structs > > are aligned. She has attached the output of pahole, so that the > > community can verify her results and hence the lengthy output. > > > > She can also cross compile the kernel and verify the output for > > other architectures using pahole. Kindly let us know if this suits > > you. And please name any specific architecture that you would to see > > tested. If this is still not enough from your point of view, let > > us know what should be done further to verify the correctness of > > the patch. > > > > Really, I hate this checkpatch.pl warning, too. The patches are > difficult to review because you need a lot of context and there is a > small chance that the patch will introduce a bug. > > I was the person who introduced the rule that the patch submitter has to > prove the alignment is correct after two people told me basically that, > "The patch submitter's job is to sed the code and the maintainer's job > is to review the code." > > In this case we don't really need to use pahole. "mac" is a 6 byte > char array declared on the stack after a couple of integers. > pnetdev->dev_addr is a pointer. &pdata[0x12] is a pointer plus an even > offset. This patch is fine. But the changelog is too long and has a > lot of not at all relevant output from pahole. > Thanks for your analysis. > It's not really a practical thing to say that the patch writer has to > cross compile on a different arch. > I was trying to make ends meet. Thanks for the advice and ruling out a very difficult option. > regards, > dan carpenter > Heba, kindly resend the patch with an adjusted description. Include the relevant blocks of any struct and state more details in the last sentence. Kind Regards, Aya Saif El-yazal Mahfouz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [HPDD-discuss] [PATCH] staging: lustre: include: lustre_update.h: Fix for possible null pointer dereference
Hello! On Jan 29, 2015, at 2:49 PM, Frank Zago wrote: @@ -165,12 +165,14 @@ static inline int update_get_reply_buf(struct update_reply *reply, void **buf, int result; ptr = update_get_buf_internal(reply, index, &size); + + LASSERT((ptr != NULL && size >= sizeof(int))); >>> >>> >>> Now size is tested before result. So it could assert if result < 0, while >>> the function would have returned before. >> >> But if prt is null krachar on the line: >> result = *(int *)ptr; >> >> Maybe there should be two LASSERT then. > > > Yes, that would be safer. Actually I just noticed this function does not appear to be used in the client code at all. As such let's just remove update_get_reply_buf()? In fat I bet this entire lustre_update.h contains server side updating code, and is unused anywhere in the client code, so we might just be able to easily remove that. I see the only includer is ./lustre/ptlrpc/layout.c that I don't think actually uses anything there? Thanks. Bye, Oleg ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8712: fix Prefer ether_addr_copy() over memcpy()
On 01/29/2015 01:54 PM, Aya Mahfouz wrote: On Thu, Jan 29, 2015 at 02:51:57PM +0300, Dan Carpenter wrote: On Wed, Jan 28, 2015 at 11:30:11PM +0200, Aya Mahfouz wrote: On Wed, Jan 28, 2015 at 10:48:40AM -0600, Larry Finger wrote: On 01/28/2015 09:53 AM, Heba Aamer wrote: This patch fixes the following checkpatch.pl warning: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2) I used the following coccinelle script: @@ expression E1,E2;constant E3; @@ - memcpy(E1, E2, E3) + ether_addr_copy(E1, E2) pahole showed that the used structs are aligned to u16. I think you can stop here. The commit message is much too long for a 2-line patch. BTW, have you tested this patch? In particular, it needs to be tested on an architecture where alignment is important. Using x86 is not sufficient. The reason I ask is that there have been a lot of patches lately that change locking and alignment issues that are only build tested, and have never been tested with real hardware on any platform. One other thing, checkpatch only suggests that this change should be made. It is certainly not mandatory. As you have not indicated that it has been tested, NACK Larry Hello Larry, Thank you for your patience. Heba has submitted this patch as part of a workshop she currently attends. She has checked the alignment through pahole and it showed that the variables of complex structs are aligned. She has attached the output of pahole, so that the community can verify her results and hence the lengthy output. She can also cross compile the kernel and verify the output for other architectures using pahole. Kindly let us know if this suits you. And please name any specific architecture that you would to see tested. If this is still not enough from your point of view, let us know what should be done further to verify the correctness of the patch. Really, I hate this checkpatch.pl warning, too. The patches are difficult to review because you need a lot of context and there is a small chance that the patch will introduce a bug. I was the person who introduced the rule that the patch submitter has to prove the alignment is correct after two people told me basically that, "The patch submitter's job is to sed the code and the maintainer's job is to review the code." In this case we don't really need to use pahole. "mac" is a 6 byte char array declared on the stack after a couple of integers. pnetdev->dev_addr is a pointer. &pdata[0x12] is a pointer plus an even offset. This patch is fine. But the changelog is too long and has a lot of not at all relevant output from pahole. Thanks for your analysis. It's not really a practical thing to say that the patch writer has to cross compile on a different arch. I was trying to make ends meet. Thanks for the advice and ruling out a very difficult option. regards, dan carpenter Heba, kindly resend the patch with an adjusted description. Include the relevant blocks of any struct and state more details in the last sentence. I agree; however, keep the commit message short. I think stating that any members of a struct have been checked with pahole will be suffifient. Obviously, the local arrays will be aligned correctly. Larry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: fid: lproc_fid: Improving error control
Improving error checking by now use a return value from sscanf. This was found using a static code analysis program called cppcheck Signed-off-by: Rickard Strandqvist --- drivers/staging/lustre/lustre/fid/lproc_fid.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/lustre/lustre/fid/lproc_fid.c b/drivers/staging/lustre/lustre/fid/lproc_fid.c index 6a21f07..9b4ada4 100644 --- a/drivers/staging/lustre/lustre/fid/lproc_fid.c +++ b/drivers/staging/lustre/lustre/fid/lproc_fid.c @@ -85,7 +85,7 @@ static int lprocfs_fid_write_common(const char __user *buffer, size_t count, rc = sscanf(kernbuf, "[%llx - %llx]\n", (unsigned long long *)&tmp.lsr_start, (unsigned long long *)&tmp.lsr_end); - if (!range_is_sane(&tmp) || range_is_zero(&tmp) || + if (rc != 2 || !range_is_sane(&tmp) || range_is_zero(&tmp) || tmp.lsr_start < range->lsr_start || tmp.lsr_end > range->lsr_end) return -EINVAL; *range = tmp; -- 1.7.10.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used
2015-01-29 3:37 GMT+01:00 Chris Rorvick :
> On Wed, Jan 28, 2015 at 4:42 PM, Rickard Strandqvist
> wrote:
>> Variable ar assigned a value that is never used.
>> I have also removed all the code that thereby serves no purpose.
>
> Each of these changes adds a warning ...
>
>> diff --git a/drivers/staging/emxx_udc/emxx_udc.c
>> b/drivers/staging/emxx_udc/emxx_udc.c
>> index eb178fc..b916fab 100644
>> --- a/drivers/staging/emxx_udc/emxx_udc.c
>> +++ b/drivers/staging/emxx_udc/emxx_udc.c
>> @@ -2974,10 +2974,10 @@ static int nbu2ss_ep_fifo_status(struct usb_ep *_ep)
>> spin_lock_irqsave(&udc->lock, flags);
>>
>> if (ep->epnum == 0) {
>> - data = _nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
>> + _nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
>
> .../linux/drivers/staging/emxx_udc/emxx_udc.c:2977:36: warning: value
> computed is not used [-Wunused-value]
>_nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
> ^
>
>> } else {
>> - data = _nbu2ss_readl(&preg->EP_REGS[ep->epnum-1].EP_LEN_DCNT)
>> + _nbu2ss_readl(&preg->EP_REGS[ep->epnum-1].EP_LEN_DCNT)
>> & EPn_LDATA;
>
> .../linux/drivers/staging/emxx_udc/emxx_udc.c:2981:4: warning: value
> computed is not used [-Wunused-value]
> & EPn_LDATA;
> ^
>
>> }
>>
>> @@ -3264,12 +3264,11 @@ static void __init nbu2ss_drv_set_ep_info(
>> if (isdigit(name[2])) {
>>
>> longnum;
>> - int res;
>> chartempbuf[2];
>>
>> tempbuf[0] = name[2];
>> tempbuf[1] = '\0';
>> - res = kstrtol(tempbuf, 16, &num);
>> + kstrtol(tempbuf, 16, &num);
>
> .../linux/drivers/staging/emxx_udc/emxx_udc.c:3271:3: warning:
> ignoring return value of ‘kstrtol’, declared with attribute
> warn_unused_result [-Wunused-result]
>kstrtol(tempbuf, 16, &num);
>^
Hi
The first two were my fault, stupid of me to let the "& number" remain :(
The last one was more interesting, se below.
But I can not really see how any error should be handled here?
Proposal to change to:
if (kstrtol(tempbuf, 16, &num) == 0 && num == 0)
static inline int __must_check kstrtol(const char *s, unsigned int
base, long *res);
"The __must_check annotation makes use of the gcc warn_unused_result
attribute; it first found its way into the mainline in 2.6.8. If a
function is marked __must_check, the compiler will issue a strong
warning whenever the function is called and its return code is unused.
"
Kind regards
Rickard Strandqvist
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
solo6x10: all interrupts for all cards handled by CPU0, no balancing - why?
Hi, having another "card freeze" issue with linux-next (tag next-20150128) on a server running 3 solo6110 cards. The freeze happens after 3 days or so. Much better than 30 minutes, which was the case before the recent enhancement by Krzysztof Halasa. This is Ubuntu Trusty. There's /usr/sbin/irqbalance process running. See interrupt stats here: https://gist.github.com/krieger-od/f8d99080d6fc30dad3d2 I wonder why all solo6x10 interrupts happen on CPU0, while there are 3 more cores. However, I have got an idea reading this: Irqbalance is a daemon to help balance the cpu load generated by interrupts across all of a systems cpus. Irqbalance identifies the highest volume interrupt sources, and isolates them to a single unique cpu, so that load is spread as much as possible over an entire processor set, while minimizing cache hit rates for irq handlers. Disabled irqbalance launch on boot. Rebooted, and the host got down up to now :( Any comments, except laugh? :) -- Andrey Utkin ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: jr3_pci: Removed variables that is never used
2015-01-29 15:26 GMT+01:00 Ian Abbott :
> On 28/01/15 22:35, Rickard Strandqvist wrote:
>>
>> Variable ar assigned a value that is never used.
>> I have also removed all the code that thereby serves no purpose.
>>
>> This was found using a static code analysis program called cppcheck
>>
>> Signed-off-by: Rickard Strandqvist
>>
>> ---
>> drivers/staging/comedi/drivers/jr3_pci.c |3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/jr3_pci.c
>> b/drivers/staging/comedi/drivers/jr3_pci.c
>> index 81fab2d..5d4cca7 100644
>> --- a/drivers/staging/comedi/drivers/jr3_pci.c
>> +++ b/drivers/staging/comedi/drivers/jr3_pci.c
>> @@ -520,10 +520,9 @@ static struct jr3_pci_poll_delay
>> jr3_pci_poll_subdevice(struct comedi_subdevice
>> result = poll_delay_min_max(20, 100);
>> } else {
>> /* Set full scale */
>> - struct six_axis_t min_full_scale;
>> struct six_axis_t max_full_scale;
>>
>> - min_full_scale = get_min_full_scales(channel);
>> + get_min_full_scales(channel);
>> max_full_scale = get_max_full_scales(channel);
>> set_full_scales(channel, max_full_scale);
>>
>>
>
> Yes, it doesn't appear to be needed. The driver used to have some kernel
> logs that output the min and max full scale information, but it didn't do
> anything else with min_full_scale.
>
> The call to get_min_full_scales() and the function itself can also be
> removed.
>
> --
> -=( Ian Abbott @ MEV Ltd.E-mail: )=-
> -=( Web: http://www.mev.co.uk/ )=-
Hi
Ok good, new patch on the way!
Kind regards
Rickard Strandqvist
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: rtl8712: fix Prefer ether_addr_copy() over memcpy()
This patch fixes the following checkpatch.pl warning:
Prefer ether_addr_copy() over memcpy()
if the Ethernet addresses are __aligned(2)
pahole showed that the struct used pnetdev->dev_addr
is aligned to u16.
Moreover mac is a simple array, pdata is a pointer that
starts from an even offset.
Signed-off-by: Heba Aamer
---
v2: modifying patch description
drivers/staging/rtl8712/usb_intf.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8712/usb_intf.c
b/drivers/staging/rtl8712/usb_intf.c
index 15f0d42..a2f2dfb 100644
--- a/drivers/staging/rtl8712/usb_intf.c
+++ b/drivers/staging/rtl8712/usb_intf.c
@@ -463,7 +463,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
/* Use the mac address stored in the Efuse
* offset = 0x12 for usb in efuse
*/
- memcpy(mac, &pdata[0x12], ETH_ALEN);
+ ether_addr_copy(mac, &pdata[0x12]);
}
eeprom_CustomerID = pdata[0x52];
switch (eeprom_CustomerID) {
@@ -580,7 +580,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
} else
dev_info(&udev->dev,
"r8712u: MAC Address from efuse = %pM\n", mac);
- memcpy(pnetdev->dev_addr, mac, ETH_ALEN);
+ ether_addr_copy(pnetdev->dev_addr, mac);
}
/* step 6. Load the firmware asynchronously */
if (rtl871x_load_fw(padapter))
--
1.7.9.5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: comedi: drivers: jr3_pci: Removed variables that is never used
Variable was assigned a value that was never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/jr3_pci.c |2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/jr3_pci.c
b/drivers/staging/comedi/drivers/jr3_pci.c
index 81fab2d..46edead 100644
--- a/drivers/staging/comedi/drivers/jr3_pci.c
+++ b/drivers/staging/comedi/drivers/jr3_pci.c
@@ -520,10 +520,8 @@ static struct jr3_pci_poll_delay
jr3_pci_poll_subdevice(struct comedi_subdevice
result = poll_delay_min_max(20, 100);
} else {
/* Set full scale */
- struct six_axis_t min_full_scale;
struct six_axis_t max_full_scale;
- min_full_scale = get_min_full_scales(channel);
max_full_scale = get_max_full_scales(channel);
set_full_scales(channel, max_full_scale);
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference
On Thu, 29 Jan 2015 19:48:08 +0100, Rickard Strandqvist said:
> Fix a possible null pointer dereference, there is
> otherwise a risk of a possible null pointer dereference.
>
> This was found using a static code analysis program called cppcheck
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/staging/media/lirc/lirc_zilog.c |4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
> /* find our IR struct */
> struct IR *ir = filep->private_data;
>
> - if (ir == NULL) {
> - dev_err(ir->l.dev, "close: no private_data attached to the
> file!\n");
Yes, the dev_err() call is an obvious thinko.
However, I'm not sure whether removing it entirely is right either. If
there *should* be a struct IR * passed there, maybe some other printk()
should be issued, or even a WARN_ON(!ir), or something?
pgpVPFpeMF6Av.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: drivers: mite: Removed variables that is never used
2015-01-29 15:39 GMT+01:00 Ian Abbott :
> On 28/01/15 22:36, Rickard Strandqvist wrote:
>>
>> Variable ar assigned a value that is never used.
>> I have also removed all the code that thereby serves no purpose.
>>
>> This was found using a static code analysis program called cppcheck
>>
>> Signed-off-by: Rickard Strandqvist
>>
>> ---
>> drivers/staging/comedi/drivers/mite.c |3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/mite.c
>> b/drivers/staging/comedi/drivers/mite.c
>> index ffc9e61..8d2903b 100644
>> --- a/drivers/staging/comedi/drivers/mite.c
>> +++ b/drivers/staging/comedi/drivers/mite.c
>> @@ -494,9 +494,8 @@ EXPORT_SYMBOL_GPL(mite_bytes_read_from_memory_ub);
>> unsigned mite_dma_tcr(struct mite_channel *mite_chan)
>> {
>> struct mite_struct *mite = mite_chan->mite;
>> - int lkar;
>>
>> - lkar = readl(mite->mite_io_addr + MITE_LKAR(mite_chan->channel));
>> + readl(mite->mite_io_addr + MITE_LKAR(mite_chan->channel));
>> return readl(mite->mite_io_addr + MITE_TCR(mite_chan->channel));
>> }
>> EXPORT_SYMBOL_GPL(mite_dma_tcr);
>>
>
> Reading the MITE_LKAR(channel) register won't have any side-effects, so that
> call to readl() can be removed altogether. In previous versions of the
> driver, the value of lkar was only used in debugging messages.
>
> --
> -=( Ian Abbott @ MEV Ltd.E-mail: )=-
> -=( Web: http://www.mev.co.uk/ )=-
Hi
Ok good, new patch on the way!
Kind regards
Rickard Strandqvist
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: comedi: drivers: mite: Removed variables that is never used
Variable was assigned a value that was never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/mite.c |2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/mite.c
b/drivers/staging/comedi/drivers/mite.c
index ffc9e61..1e537a5 100644
--- a/drivers/staging/comedi/drivers/mite.c
+++ b/drivers/staging/comedi/drivers/mite.c
@@ -494,9 +494,7 @@ EXPORT_SYMBOL_GPL(mite_bytes_read_from_memory_ub);
unsigned mite_dma_tcr(struct mite_channel *mite_chan)
{
struct mite_struct *mite = mite_chan->mite;
- int lkar;
- lkar = readl(mite->mite_io_addr + MITE_LKAR(mite_chan->channel));
return readl(mite->mite_io_addr + MITE_TCR(mite_chan->channel));
}
EXPORT_SYMBOL_GPL(mite_dma_tcr);
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: comedi: drivers: addi-data: hwdrv_apci3501: Removed variables that is never used
Variable was assigned a value that was never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c
b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c
index 339519a..1f2f781 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3501.c
@@ -93,7 +93,6 @@ static int apci3501_write_insn_timer(struct comedi_device
*dev,
{
struct apci3501_private *devpriv = dev->private;
unsigned int ul_Command1 = 0;
- int i_Temp;
if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
@@ -135,7 +134,7 @@ static int apci3501_write_insn_timer(struct comedi_device
*dev,
}
}
- i_Temp = inl(dev->iobase + APCI3501_TIMER_STATUS_REG) & 0x1;
+ inl(dev->iobase + APCI3501_TIMER_STATUS_REG);
return insn->n;
}
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: solo6x10: all interrupts for all cards handled by CPU0, no balancing - why?
The host was rebooted and got back online. Without irqbalance daemon, all solo6x10 interrupts are still on CPU0. See https://gist.github.com/krieger-od/d1686243c67fbe3e14a5 Any ideas are strongly appreciated. -- Andrey Utkin ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: comedi: drivers: addi_apci_3501: Removed variables that is never used
Variable was assigned a value that was never used. I have also removed all the code that thereby serves no purpose. This was found using a static code analysis program called cppcheck Signed-off-by: Rickard Strandqvist --- drivers/staging/comedi/drivers/addi_apci_3501.c |3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/comedi/drivers/addi_apci_3501.c b/drivers/staging/comedi/drivers/addi_apci_3501.c index a726efc..5961f19 100644 --- a/drivers/staging/comedi/drivers/addi_apci_3501.c +++ b/drivers/staging/comedi/drivers/addi_apci_3501.c @@ -267,7 +267,6 @@ static irqreturn_t apci3501_interrupt(int irq, void *d) struct apci3501_private *devpriv = dev->private; unsigned int ui_Timer_AOWatchdog; unsigned long ul_Command1; - int i_temp; /* Disable Interrupt */ ul_Command1 = inl(dev->iobase + APCI3501_TIMER_CTRL_REG); @@ -285,7 +284,7 @@ static irqreturn_t apci3501_interrupt(int irq, void *d) ul_Command1 = inl(dev->iobase + APCI3501_TIMER_CTRL_REG); ul_Command1 = ((ul_Command1 & 0xF9FDul) | 1 << 1); outl(ul_Command1, dev->iobase + APCI3501_TIMER_CTRL_REG); - i_temp = inl(dev->iobase + APCI3501_TIMER_STATUS_REG) & 0x1; + inl(dev->iobase + APCI3501_TIMER_STATUS_REG); return IRQ_HANDLED; } -- 1.7.10.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: comedi: drivers: dyna_pci10xx: Removed variables that is never used
Variable was assigned a value that was never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/dyna_pci10xx.c |6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dyna_pci10xx.c
b/drivers/staging/comedi/drivers/dyna_pci10xx.c
index 1b6324c..56490f4 100644
--- a/drivers/staging/comedi/drivers/dyna_pci10xx.c
+++ b/drivers/staging/comedi/drivers/dyna_pci10xx.c
@@ -1,4 +1,4 @@
-/*
+_/*
* comedi/drivers/dyna_pci10xx.c
* Copyright (C) 2011 Prashant Shah, pshah.mum...@gmail.com
*
@@ -115,10 +115,6 @@ static int dyna_pci10xx_insn_write_ao(struct comedi_device
*dev,
{
struct dyna_pci10xx_private *devpriv = dev->private;
int n;
- unsigned int chan, range;
-
- chan = CR_CHAN(insn->chanspec);
- range = range_codes_pci1050_ai[CR_RANGE((insn->chanspec))];
mutex_lock(&devpriv->mutex);
for (n = 0; n < insn->n; n++) {
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: comedi: drivers: rtd520: Removed variables that is never used
Variable was assigned a value that was never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/rtd520.c |4
1 file changed, 4 deletions(-)
diff --git a/drivers/staging/comedi/drivers/rtd520.c
b/drivers/staging/comedi/drivers/rtd520.c
index 581aa58..1130bf0 100644
--- a/drivers/staging/comedi/drivers/rtd520.c
+++ b/drivers/staging/comedi/drivers/rtd520.c
@@ -1031,8 +1031,6 @@ static int rtd_ai_cmd(struct comedi_device *dev, struct
comedi_subdevice *s)
static int rtd_ai_cancel(struct comedi_device *dev, struct comedi_subdevice *s)
{
struct rtd_private *devpriv = dev->private;
- u32 overrun;
- u16 status;
/* pacer stop source: SOFTWARE */
writel(0, dev->mmio + LAS0_PACER_STOP);
@@ -1040,8 +1038,6 @@ static int rtd_ai_cancel(struct comedi_device *dev,
struct comedi_subdevice *s)
writel(0, dev->mmio + LAS0_ADC_CONVERSION);
writew(0, dev->mmio + LAS0_IT);
devpriv->ai_count = 0; /* stop and don't transfer any more */
- status = readw(dev->mmio + LAS0_IT);
- overrun = readl(dev->mmio + LAS0_OVERRUN) & 0x;
writel(0, dev->mmio + LAS0_ADC_FIFO_CLEAR);
return 0;
}
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: comedi: drivers: usbduxsigma: Removed variables that is never used
Variable was assigned a value that was never used.
I have also removed all the code that thereby serves no purpose.
This was found using a static code analysis program called cppcheck
Signed-off-by: Rickard Strandqvist
---
drivers/staging/comedi/drivers/usbduxsigma.c |4
1 file changed, 4 deletions(-)
diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c
b/drivers/staging/comedi/drivers/usbduxsigma.c
index dc19435..185f2d1 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -215,7 +215,6 @@ static void usbduxsigma_ai_handle_urb(struct comedi_device
*dev,
struct usbduxsigma_private *devpriv = dev->private;
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = &async->cmd;
- unsigned int dio_state;
uint32_t val;
int ret;
int i;
@@ -224,9 +223,6 @@ static void usbduxsigma_ai_handle_urb(struct comedi_device
*dev,
if (devpriv->ai_counter == 0) {
devpriv->ai_counter = devpriv->ai_timer;
- /* get the state of the dio pins to allow external trigger */
- dio_state = be32_to_cpu(devpriv->in_buf[0]);
-
/* get the data from the USB bus and hand it over to comedi */
for (i = 0; i < cmd->chanlist_len; i++) {
/* transfer data, note first byte is the DIO state */
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: lustre: osc: fix Prefer seq_puts to seq_printf
On Wed, Jan 28, 2015 at 05:56:07PM -0800, Joe Perches wrote: > On Wed, 2015-01-28 at 16:05 +0200, Heba Aamer wrote: > > This patch fixes the following checkpatch.pl warning: > > Prefer seq_puts to seq_printf > > checkpatch is pretty stupid. > Please don't just do what it says. I checked checkpatch script and I found it only searches for a % in order not to give that warning, and it does not consider its escaping case %%. > > Look further and see what else can be improved. I will make a new patch modifying all the seq_printf statements in that file soon. Regards, Heba Aamer > > > diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c > > b/drivers/staging/lustre/lustre/osc/lproc_osc.c > [] > > @@ -364,7 +364,7 @@ static int osc_checksum_type_seq_show(struct seq_file > > *m, void *v) > > else > > seq_printf(m, "%s ", cksum_name[i]); > > } > > - seq_printf(m, "\n"); > > + seq_puts(m, "\n"); > > This could be seq_putc > > > @@ -601,7 +601,7 @@ static int osc_rpc_stats_seq_show(struct seq_file *seq, > > void *v) > > seq_printf(seq, "pending read pages: %d\n", > >atomic_read(&cli->cl_pending_r_pages)); > > > > - seq_printf(seq, "\n\t\t\tread\t\t\twrite\n"); > > + seq_puts(seq, "\n\t\t\tread\t\t\twrite\n"); > > seq_printf(seq, "pages per rpc rpcs %% cum %% |"); > > seq_printf(seq, " rpcs %% cum %%\n"); > > The seq_printf uses with %% could also be seq_puts > > > @@ -624,7 +624,7 @@ static int osc_rpc_stats_seq_show(struct seq_file *seq, > > void *v) > > break; > > } > > > > - seq_printf(seq, "\n\t\t\tread\t\t\twrite\n"); > > + seq_puts(seq, "\n\t\t\tread\t\t\twrite\n"); > > seq_printf(seq, "rpcs in flight rpcs %% cum %% |"); > > seq_printf(seq, " rpcs %% cum %%\n"); > > seq_puts here too > > > @@ -647,7 +647,7 @@ static int osc_rpc_stats_seq_show(struct seq_file *seq, > > void *v) > > break; > > } > > > > - seq_printf(seq, "\n\t\t\tread\t\t\twrite\n"); > > + seq_puts(seq, "\n\t\t\tread\t\t\twrite\n"); > > seq_printf(seq, "offset rpcs %% cum %% |"); > > seq_printf(seq, " rpcs %% cum %%\n"); > > > > and here > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: lustre: osc: modifying seq_printf statements
This patch modifies the seq_printf statements in drivers/staging/lustre/lustre/osc/lproc_osc.c file. It changes it to seq_puts and seq_putc wherever applicable. Signed-off-by: Heba Aamer --- drivers/staging/lustre/lustre/osc/lproc_osc.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c index 8e22e45..61d2ca4 100644 --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c @@ -364,7 +364,7 @@ static int osc_checksum_type_seq_show(struct seq_file *m, void *v) else seq_printf(m, "%s ", cksum_name[i]); } - seq_printf(m, "\n"); + seq_putc(m, '\n'); return 0; } @@ -601,9 +601,9 @@ static int osc_rpc_stats_seq_show(struct seq_file *seq, void *v) seq_printf(seq, "pending read pages: %d\n", atomic_read(&cli->cl_pending_r_pages)); - seq_printf(seq, "\n\t\t\tread\t\t\twrite\n"); - seq_printf(seq, "pages per rpc rpcs %% cum %% |"); - seq_printf(seq, " rpcs %% cum %%\n"); + seq_puts(seq, "\n\t\t\tread\t\t\twrite\n"); + seq_puts(seq, "pages per rpc rpcs %% cum %% |"); + seq_puts(seq, " rpcs %% cum %%\n"); read_tot = lprocfs_oh_sum(&cli->cl_read_page_hist); write_tot = lprocfs_oh_sum(&cli->cl_write_page_hist); @@ -624,9 +624,9 @@ static int osc_rpc_stats_seq_show(struct seq_file *seq, void *v) break; } - seq_printf(seq, "\n\t\t\tread\t\t\twrite\n"); - seq_printf(seq, "rpcs in flight rpcs %% cum %% |"); - seq_printf(seq, " rpcs %% cum %%\n"); + seq_puts(seq, "\n\t\t\tread\t\t\twrite\n"); + seq_puts(seq, "rpcs in flight rpcs %% cum %% |"); + seq_puts(seq, " rpcs %% cum %%\n"); read_tot = lprocfs_oh_sum(&cli->cl_read_rpc_hist); write_tot = lprocfs_oh_sum(&cli->cl_write_rpc_hist); @@ -647,9 +647,9 @@ static int osc_rpc_stats_seq_show(struct seq_file *seq, void *v) break; } - seq_printf(seq, "\n\t\t\tread\t\t\twrite\n"); - seq_printf(seq, "offset rpcs %% cum %% |"); - seq_printf(seq, " rpcs %% cum %%\n"); + seq_puts(seq, "\n\t\t\tread\t\t\twrite\n"); + seq_puts(seq, "offset rpcs %% cum %% |"); + seq_puts(seq, " rpcs %% cum %%\n"); read_tot = lprocfs_oh_sum(&cli->cl_read_offset_hist); write_tot = lprocfs_oh_sum(&cli->cl_write_offset_hist); -- 1.7.9.5 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used
On Thu, Jan 29, 2015 at 3:52 PM, Rickard Strandqvist
wrote:
> The last one was more interesting, se below.
> But I can not really see how any error should be handled here?
> Proposal to change to:
>
> if (kstrtol(tempbuf, 16, &num) == 0 && num == 0)
That whole chunk of code looks odd. Why the base 16 conversion when
we already know it's a decimal digit? Seems like this would work
without the hassle of the string conversion:
-- >8 --
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -3262,16 +3262,7 @@ static void __init nbu2ss_drv_set_ep_info(
ep->ep.ops = &nbu2ss_ep_ops;
if (isdigit(name[2])) {
-
- longnum;
- int res;
- chartempbuf[2];
-
- tempbuf[0] = name[2];
- tempbuf[1] = '\0';
- res = kstrtol(tempbuf, 16, &num);
-
- if (num == 0)
+ if (name[2] == '0')
ep->ep.maxpacket = EP0_PACKETSIZE;
else
ep->ep.maxpacket = EP_PACKETSIZE;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK
On Thu, Jan 15, 2015 at 9:03 AM, Michal Hocko wrote: > On Mon 12-01-15 21:49:14, Chintan Pandya wrote: >> The global shrinker will invoke lowmem_shrink in a loop. >> The loop will be run (total_scan_pages/batch_size) times. >> The default batch_size will be 128 which will make >> shrinker invoking 100s of times. LMK does meaningful >> work only during first 2-3 times and then rest of the >> invocations are just CPU cycle waste. Fix that by returning >> to the shrinker with SHRINK_STOP when LMK doesn't find any >> more work to do. The deciding factor here is, no process >> found in the selected LMK bucket or memory conditions are >> sane. > > lowmemory killer is broken by design and this one of the examples which > shows why. It simply doesn't fit into shrinkers concept. > > The count_object callback simply lies and tells the core that all > the reclaimable LRU pages are scanable and gives it this as a number > which the core uses for total_scan. scan_objects callback then happily > ignore nr_to_reclaim and does its one time job where it iterates over > _all_ tasks and picks up the victim and returns its rss as a return > value. This is just a subset of LRU pages of course so it continues > looping until total_scan goes down to 0 finally. > > If this really has to be a shrinker then, shouldn't it evaluate the OOM > situation in the count callback and return non zero only if OOM and then > the scan callback would kill and return nr_to_reclaim. > > Or even better wouldn't it be much better to use vmpressure to wake > up a kernel module which would simply check the situation and kill > something? > > Please do not put only cosmetic changes on top of broken concept and try > to think about a proper solution that is what staging is for AFAIU. > > The code is in this state for quite some time and I would really hate if > it got merged just because it is in staging for too long and it is used > out there. So the in-kernel low-memory-killer is hopefully on its way out. With Lollipop on some devices, Android is using the mempressure notifiers to kill processes from userland. However, not all devices have moved to this new model (and possibly some resulting performance issues are being worked out? Its not clear). So hopefully we can drop it soon, but I'd like to make sure we don't get only a half-working solution upstream before we do remove it. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used
On Thu, Jan 29, 2015 at 5:46 PM, Chris Rorvick wrote:
> That whole chunk of code looks odd. Why the base 16 conversion when
> we already know it's a decimal digit? Seems like this would work
> without the hassle of the string conversion:
Hmm, or probably even better to do this where the ep0 check is less contorted.
-- >8 --
diff --git a/drivers/staging/emxx_udc/emxx_udc.c
b/drivers/staging/emxx_udc/emxx_udc.c
index eb178fc..98a1ace 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -3261,25 +3261,6 @@ static void __init nbu2ss_drv_set_ep_info(
ep->ep.name = name;
ep->ep.ops = &nbu2ss_ep_ops;
- if (isdigit(name[2])) {
-
- longnum;
- int res;
- chartempbuf[2];
-
- tempbuf[0] = name[2];
- tempbuf[1] = '\0';
- res = kstrtol(tempbuf, 16, &num);
-
- if (num == 0)
- ep->ep.maxpacket = EP0_PACKETSIZE;
- else
- ep->ep.maxpacket = EP_PACKETSIZE;
-
- } else {
- ep->ep.maxpacket = EP_PACKETSIZE;
- }
-
list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
INIT_LIST_HEAD(&ep->queue);
}
@@ -3293,8 +3274,12 @@ static void __init nbu2ss_drv_ep_init(struct
nbu2ss_udc *udc)
udc->gadget.ep0 = &udc->ep[0].ep;
- for (i = 0; i < NUM_ENDPOINTS; i++)
- nbu2ss_drv_set_ep_info(udc, &udc->ep[i], gp_ep_name[i]);
+ for (i = 0; i < NUM_ENDPOINTS; i++) {
+ struct nbu2ss_ep *ep = &udc->ep[i];
+
+ nbu2ss_drv_set_ep_info(udc, ep, gp_ep_name[i]);
+ ep->ep.maxpacket = i == 0 ? EP0_PACKETSIZE : EP_PACKETSIZE;
+ }
list_del_init(&udc->ep[0].ep.ep_list);
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: rtl8712: fix Prefer ether_addr_copy() over memcpy()
On 01/29/2015 04:11 PM, Heba Aamer wrote:
This patch fixes the following checkpatch.pl warning:
Prefer ether_addr_copy() over memcpy()
if the Ethernet addresses are __aligned(2)
pahole showed that the struct used pnetdev->dev_addr
is aligned to u16.
Moreover mac is a simple array, pdata is a pointer that
starts from an even offset.
Signed-off-by: Heba Aamer
---
v2: modifying patch description
drivers/staging/rtl8712/usb_intf.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
This is much better.
Acked-by: Larry Finger
Larry
diff --git a/drivers/staging/rtl8712/usb_intf.c
b/drivers/staging/rtl8712/usb_intf.c
index 15f0d42..a2f2dfb 100644
--- a/drivers/staging/rtl8712/usb_intf.c
+++ b/drivers/staging/rtl8712/usb_intf.c
@@ -463,7 +463,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
/* Use the mac address stored in the Efuse
* offset = 0x12 for usb in efuse
*/
- memcpy(mac, &pdata[0x12], ETH_ALEN);
+ ether_addr_copy(mac, &pdata[0x12]);
}
eeprom_CustomerID = pdata[0x52];
switch (eeprom_CustomerID) {
@@ -580,7 +580,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
} else
dev_info(&udev->dev,
"r8712u: MAC Address from efuse = %pM\n", mac);
- memcpy(pnetdev->dev_addr, mac, ETH_ALEN);
+ ether_addr_copy(pnetdev->dev_addr, mac);
}
/* step 6. Load the firmware asynchronously */
if (rtl871x_load_fw(padapter))
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK
On Thu, Jan 29, 2015 at 4:44 PM, John Stultz wrote: > On Thu, Jan 15, 2015 at 9:03 AM, Michal Hocko wrote: >> On Mon 12-01-15 21:49:14, Chintan Pandya wrote: >>> The global shrinker will invoke lowmem_shrink in a loop. >>> The loop will be run (total_scan_pages/batch_size) times. >>> The default batch_size will be 128 which will make >>> shrinker invoking 100s of times. LMK does meaningful >>> work only during first 2-3 times and then rest of the >>> invocations are just CPU cycle waste. Fix that by returning >>> to the shrinker with SHRINK_STOP when LMK doesn't find any >>> more work to do. The deciding factor here is, no process >>> found in the selected LMK bucket or memory conditions are >>> sane. >> >> lowmemory killer is broken by design and this one of the examples which >> shows why. It simply doesn't fit into shrinkers concept. >> >> The count_object callback simply lies and tells the core that all >> the reclaimable LRU pages are scanable and gives it this as a number >> which the core uses for total_scan. scan_objects callback then happily >> ignore nr_to_reclaim and does its one time job where it iterates over >> _all_ tasks and picks up the victim and returns its rss as a return >> value. This is just a subset of LRU pages of course so it continues >> looping until total_scan goes down to 0 finally. >> >> If this really has to be a shrinker then, shouldn't it evaluate the OOM >> situation in the count callback and return non zero only if OOM and then >> the scan callback would kill and return nr_to_reclaim. >> >> Or even better wouldn't it be much better to use vmpressure to wake >> up a kernel module which would simply check the situation and kill >> something? >> >> Please do not put only cosmetic changes on top of broken concept and try >> to think about a proper solution that is what staging is for AFAIU. >> >> The code is in this state for quite some time and I would really hate if >> it got merged just because it is in staging for too long and it is used >> out there. > > So the in-kernel low-memory-killer is hopefully on its way out. > > With Lollipop on some devices, Android is using the mempressure > notifiers to kill processes from userland. However, not all devices > have moved to this new model (and possibly some resulting performance > issues are being worked out? Its not clear). So hopefully we can drop > it soon, but I'd like to make sure we don't get only a half-working > solution upstream before we do remove it. > > thanks > -john We are still working on a user space replacement to LMK. We have definitely had issues with LMKd and so stayed with the in kernel one for all the lollipop devices we shipped. Issues were mostly related to performance, timing of OOM notifications and when under intense memory pressure we ran into issues where even opening a file would fail due to no RAM being available. As John said, it's WIP and hopefully we'll be able to drop the in kernel one soon. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] hv: vmbus_post_msg: retry the hypercall on HV_STATUS_INVALID_CONNECTION_ID
On Thu, Jan 29, 2015 at 7:02 PM, Dexuan Cui wrote:
I got the hypercall error code on Hyper-V 2008 R2 when keeping running
"rmmod hv_netvsc; modprobe hv_netvsc; rmmod hv_utils; modprobe
hv_utils"
in a Linux guest.
Without the patch, the driver can occasionally fail to load.
CC: "K. Y. Srinivasan"
Signed-off-by: Dexuan Cui
---
arch/x86/include/uapi/asm/hyperv.h | 1 +
drivers/hv/connection.c| 9 +
2 files changed, 10 insertions(+)
diff --git a/arch/x86/include/uapi/asm/hyperv.h
b/arch/x86/include/uapi/asm/hyperv.h
index 90c458e..b9daffb 100644
--- a/arch/x86/include/uapi/asm/hyperv.h
+++ b/arch/x86/include/uapi/asm/hyperv.h
@@ -225,6 +225,7 @@
#define HV_STATUS_INVALID_HYPERCALL_CODE 2
#define HV_STATUS_INVALID_HYPERCALL_INPUT 3
#define HV_STATUS_INVALID_ALIGNMENT4
+#define HV_STATUS_INVALID_CONNECTION_ID18
#define HV_STATUS_INSUFFICIENT_BUFFERS 19
typedef struct _HV_REFERENCE_TSC_PAGE {
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index c4acd1c..8bd05f3 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -440,6 +440,15 @@ int vmbus_post_msg(void *buffer, size_t buflen)
ret = hv_post_message(conn_id, 1, buffer, buflen);
switch (ret) {
+ case HV_STATUS_INVALID_CONNECTION_ID:
+ /*
+* We could get this if we send messages too
+* frequently or the host is under low resource
+* conditions: let's wait 1 more second before
+* retrying the hypercall.
+*/
The name HV_STATUS_INVALID_CONNECTION_ID is really confusing in this
case. Since it does not show any meaning of lacking resources.
+ msleep(1000);
+ break;
case HV_STATUS_INSUFFICIENT_BUFFERS:
ret = -ENOMEM;
I thought host should return this error value when lacking resources?
case -ENOMEM:
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer
> -Original Message- > From: Jason Wang [mailto:jasow...@redhat.com] > Sent: Thursday, January 29, 2015 18:09 PM > To: Dexuan Cui > Cc: Vitaly Kuznetsov; KY Srinivasan; de...@linuxdriverproject.org; Haiyang > Zhang; > linux-ker...@vger.kernel.org; Radim Krčmář; Dan Carpenter > Subject: RE: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind > offer > > > > On Wed, Jan 28, 2015 at 8:51 PM, Dexuan Cui wrote: > >> -Original Message- > >> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > >> Sent: Wednesday, January 28, 2015 20:09 PM > >> To: Dexuan Cui > >> Cc: KY Srinivasan; de...@linuxdriverproject.org; Haiyang Zhang; > >> linux- > >> ker...@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter > >> Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and > >> Rescind > >> offer > >> > >> Dexuan Cui writes: > >> > >> >> -Original Message- > >> >> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > >> >> Sent: Tuesday, January 20, 2015 23:45 PM > >> >> To: KY Srinivasan; de...@linuxdriverproject.org > >> >> Cc: Haiyang Zhang; linux-ker...@vger.kernel.org; Dexuan Cui; > >> Jason Wang; > >> >> Radim Krčmář; Dan Carpenter > >> >> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and > >> Rescind > >> offer > >> ... > >> > > >> > Hi Vitaly and all, > >> > I have 2 questions: > >> > In vmbus_process_offer(), in the cases of "goto err_free_chan", > >> > should we consider the possibility a rescind message could be > >> pending for > >> > the new channel? > >> > In the cases, because we don't run > >> > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ", > >> > vmbus_onoffer_rescind() will do nothing and as a result, > >> > vmbus_process_rescind_offer() won't be invoked. > >> > >> Yes, but processing the rescind offer results in freeing the channel > >> (and this processing supposes the channel wasn't freed before) so > >> there is no difference... or is it? > >> > >> > > >> > Question 2: in vmbus_process_offer(), in the case > >> > vmbus_device_register() fails, we'll run > >> > "list_del(&newchannel->listentry);" -- just after this line, > >> > what will happen at this time if relid2channel() returns NULL > >> > in vmbus_onoffer_rescind()? > >> > > >> > I think we'll lose the rescind message. > >> > > >> > >> Yes, but same logic applies - we already freed the channes so no > >> rescind > >> proccessing required. > > free_channel() and vmbus_process_rescind_offer() are different, > > because > > the latter does more work, e.g., sending the host a message > > CHANNELMSG_RELID_RELEASED. > > > > In the cases of "goto err_free_chan" + "a pending rescind message", > > the host may expect the message CHANNELMSG_RELID_RELEASED and > > could reoffer the channel once the message is received. > > > > It would be better if the VM doesn't lose the rescind message here. > > :-) > > It's interesting that rescind needs a ack from guest. But looks like > the offer does not need it? Is there a spec for this for us for > reference? My understanding is: The host may reuse the same relid after the VM acks the rescind message. I don't have a VMBus spec either. > > > > > >> If we still need to do something we need to > >> add support for already freed channel to the rescind offer > >> processing path. > >> This sounds reasonable to me. Error handling is always full of various corner cases... Thanks, -- Dexuan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 1/3] hv: hv_util: move vmbus_open() to a later place
> -Original Message- > From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > Sent: Thursday, January 29, 2015 21:13 PM > To: Dexuan Cui > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; driverdev- > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; > jasow...@redhat.com; KY Srinivasan; Haiyang Zhang > Subject: Re: [PATCH 1/3] hv: hv_util: move vmbus_open() to a later place > > Dexuan Cui writes: > > > Before the line vmbus_open() returns, srv->util_cb can be already running > > and the variablies, like util_fw_version, are needed by the srv->util_cb. > > > > So we have to make sure the variables are initialized before the > > vmbus_open(). > > > > CC: "K. Y. Srinivasan" > > Signed-off-by: Dexuan Cui > > It is not said in the description but moving hv_set_drvdata() before > vmbus_open() make sense in case probe and remove can collide (can > they?). I'm not so sure. In normal usages, the collision should be unlikely. Anyway, the patch makes things better. :-) > > Reviewed-by: Vitaly Kuznetsov Thanks for the review! Thanks, -- Dexuan > > --- > > drivers/hv/hv_util.c | 11 ++- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c > > index 3b9c9ef..c5be773 100644 > > --- a/drivers/hv/hv_util.c > > +++ b/drivers/hv/hv_util.c > > @@ -340,12 +340,8 @@ static int util_probe(struct hv_device *dev, > > > > set_channel_read_state(dev->channel, false); > > > > - ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0, > > - srv->util_cb, dev->channel); > > - if (ret) > > - goto error; > > - > > hv_set_drvdata(dev, srv); > > + > > /* > > * Based on the host; initialize the framework and > > * service version numbers we will negotiate. > > @@ -365,6 +361,11 @@ static int util_probe(struct hv_device *dev, > > hb_srv_version = HB_VERSION; > > } > > > > + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0, > > + srv->util_cb, dev->channel); > > + if (ret) > > + goto error; > > + > > return 0; > > > > error: > > -- > Vitaly ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
> -Original Message-
> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
> Sent: Thursday, January 29, 2015 21:22 PM
> To: Dexuan Cui
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; driverdev-
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> jasow...@redhat.com; KY Srinivasan; Haiyang Zhang
> Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
>
> Dexuan Cui writes:
>
> > Without this patch, the state is put to CHANNEL_OPENING_STATE, and when
> > the driver is loaded next time, vmbus_open() will fail immediately due to
> > newchannel->state != CHANNEL_OPEN_STATE.
>
> The patch makes sense, but I have one small doubt. We call vmbus_open
> from probe functions of various devices. E.g. in hyperv-keyboard we
> have:
> error = vmbus_open(...)
> if (error)
> goto err_free_mem;
>
> and we don't call vmbus_close(...) on this path so no
> CHANNELMSG_CLOSECHANNEL will be send.
Exactly.
> Who's gonna retry probe?
The user can try 'rmmod' and 'modprobe' the module to re-probe.
> Wouldn't it be better to close the channel?
Good question!
In your example, due to " goto err_free_mem", we don't run
vmbus_close(), so the memory allocated for the ringbuffer is actually leaked!
Next time when we reload the module, vmbus_open() will allocate new
memory for the ringbuffer.
KY, Haiyang,
Can you please confirm this issue?
Thanks,
-- Dexuan
>
> >
> > CC: "K. Y. Srinivasan"
> > Signed-off-by: Dexuan Cui
> > ---
> > drivers/hv/channel.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 2978f5e..26dcf26 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel,
> u32 send_ringbuffer_size,
> > out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
> > get_order(send_ringbuffer_size + recv_ringbuffer_size));
> >
> > - if (!out)
> > - return -ENOMEM;
> > -
> > + if (!out) {
> > + err = -ENOMEM;
> > + goto error0;
> > + }
> >
> > in = (void *)((unsigned long)out + send_ringbuffer_size);
> >
> > @@ -199,6 +200,7 @@ error0:
> > free_pages((unsigned long)out,
> > get_order(send_ringbuffer_size + recv_ringbuffer_size));
> > kfree(open_info);
> > + newchannel->state = CHANNEL_OPEN_STATE;
> > return err;
> > }
> > EXPORT_SYMBOL_GPL(vmbus_open);
>
> --
> Vitaly
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 2/3] hv: vmbus_post_msg: retry the hypercall on HV_STATUS_INVALID_CONNECTION_ID
> -Original Message-
> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
> Sent: Thursday, January 29, 2015 21:31 PM
> To: Dexuan Cui
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; driverdev-
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> jasow...@redhat.com; KY Srinivasan; Haiyang Zhang
> Subject: Re: [PATCH 2/3] hv: vmbus_post_msg: retry the hypercall on
> HV_STATUS_INVALID_CONNECTION_ID
>
> Dexuan Cui writes:
>
> > I got the hypercall error code on Hyper-V 2008 R2 when keeping running
> > "rmmod hv_netvsc; modprobe hv_netvsc; rmmod hv_utils; modprobe hv_utils"
> > in a Linux guest.
> >
> > Without the patch, the driver can occasionally fail to load.
> >
> > CC: "K. Y. Srinivasan"
> > Signed-off-by: Dexuan Cui
> > ---
> > arch/x86/include/uapi/asm/hyperv.h | 1 +
> > drivers/hv/connection.c| 9 +
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/arch/x86/include/uapi/asm/hyperv.h
> b/arch/x86/include/uapi/asm/hyperv.h
> > index 90c458e..b9daffb 100644
> > --- a/arch/x86/include/uapi/asm/hyperv.h
> > +++ b/arch/x86/include/uapi/asm/hyperv.h
> > @@ -225,6 +225,7 @@
> > #define HV_STATUS_INVALID_HYPERCALL_CODE 2
> > #define HV_STATUS_INVALID_HYPERCALL_INPUT 3
> > #define HV_STATUS_INVALID_ALIGNMENT4
> > +#define HV_STATUS_INVALID_CONNECTION_ID18
> > #define HV_STATUS_INSUFFICIENT_BUFFERS 19
>
> The gap beween 4 and 18 tells me there are other codes here ;-) Are they
> all 'permanent failures'?
It looks we only need to care about these error codes here.
BTW, you can get all the hypercall error codes in the top level functional spec:
http://blogs.msdn.com/b/virtual_pc_guy/archive/2014/02/17/updated-hypervisor-top-level-functional-specification.aspx
For this hypercall (0x005c), see "14.9.7 HvPostMessage".
> >
> > typedef struct _HV_REFERENCE_TSC_PAGE {
> > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> > index c4acd1c..8bd05f3 100644
> > --- a/drivers/hv/connection.c
> > +++ b/drivers/hv/connection.c
> > @@ -440,6 +440,15 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> > ret = hv_post_message(conn_id, 1, buffer, buflen);
> >
> > switch (ret) {
> > + case HV_STATUS_INVALID_CONNECTION_ID:
> > + /*
> > +* We could get this if we send messages too
> > +* frequently or the host is under low resource
> > +* conditions: let's wait 1 more second before
> > +* retrying the hypercall.
> > +*/
> > + msleep(1000);
> > + break;
>
> In case it is our last try (No. 10) we will return '18' from the
> function. I suggest we set ret = -ENOMEM here as well.
Thanks for the suggestion!
I think it would be better to add this to the case
HV_STATUS_INVALID_CONNECTION_ID:
ret = -EAGAIN;
?
> > case HV_STATUS_INSUFFICIENT_BUFFERS:
> > ret = -ENOMEM;
>
> Or should we treat these two equally? There is a smaller (100ms) sleep
> between tries already, we can consider changing it instead.
>
> > case -ENOMEM:
>
> --
> Vitaly
In my experiments, in the HV_STATUS_INVALID_CONNECTION_ID case,
waiting 100ms is not enough sometimes, so I'd like to wait more time.
I agree with you both cases can wait 1000ms. I'll update my patch.
BTW, the " case -ENOMEM:" is not reachable(the hypervisor itself doesn't
return -ENOMEM), I think. I can remove it.
Thanks,
-- Dexuan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 2/3] hv: vmbus_post_msg: retry the hypercall on HV_STATUS_INVALID_CONNECTION_ID
> -Original Message-
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Friday, January 30, 2015 10:47 AM
> To: Dexuan Cui
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; driverdev-
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; KY
> Srinivasan; Haiyang Zhang
> Subject: Re: [PATCH 2/3] hv: vmbus_post_msg: retry the hypercall on
> HV_STATUS_INVALID_CONNECTION_ID
>
>
>
> On Thu, Jan 29, 2015 at 7:02 PM, Dexuan Cui wrote:
> > I got the hypercall error code on Hyper-V 2008 R2 when keeping running
> > "rmmod hv_netvsc; modprobe hv_netvsc; rmmod hv_utils; modprobe
> > hv_utils"
> > in a Linux guest.
> >
> > Without the patch, the driver can occasionally fail to load.
> >
> > CC: "K. Y. Srinivasan"
> > Signed-off-by: Dexuan Cui
> > ---
> > arch/x86/include/uapi/asm/hyperv.h | 1 +
> > drivers/hv/connection.c| 9 +
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/arch/x86/include/uapi/asm/hyperv.h
> > b/arch/x86/include/uapi/asm/hyperv.h
> > index 90c458e..b9daffb 100644
> > --- a/arch/x86/include/uapi/asm/hyperv.h
> > +++ b/arch/x86/include/uapi/asm/hyperv.h
> > @@ -225,6 +225,7 @@
> > #define HV_STATUS_INVALID_HYPERCALL_CODE 2
> > #define HV_STATUS_INVALID_HYPERCALL_INPUT 3
> > #define HV_STATUS_INVALID_ALIGNMENT4
> > +#define HV_STATUS_INVALID_CONNECTION_ID18
> > #define HV_STATUS_INSUFFICIENT_BUFFERS 19
> >
> > typedef struct _HV_REFERENCE_TSC_PAGE {
> > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> > index c4acd1c..8bd05f3 100644
> > --- a/drivers/hv/connection.c
> > +++ b/drivers/hv/connection.c
> > @@ -440,6 +440,15 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> > ret = hv_post_message(conn_id, 1, buffer, buflen);
> >
> > switch (ret) {
> > + case HV_STATUS_INVALID_CONNECTION_ID:
> > + /*
> > +* We could get this if we send messages too
> > +* frequently or the host is under low resource
> > +* conditions: let's wait 1 more second before
> > +* retrying the hypercall.
> > +*/
>
> The name HV_STATUS_INVALID_CONNECTION_ID is really confusing in this
> case. Since it does not show any meaning of lacking resources.
The description about the 'low host resource condition' might be not accurate.
I'll remove this part.
> > + msleep(1000);
> > + break;
> > case HV_STATUS_INSUFFICIENT_BUFFERS:
> > ret = -ENOMEM;
>
> I thought host should return this error value when lacking resources?
This should be correct.
> >
> > case -ENOMEM:
> > --
Thanks,
-- Dexuan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
