Re: [PATCH] android: fix warning when releasing active sync point
On Mon, Dec 14, 2015 at 05:29:55PM -0800, Dmitry Torokhov wrote:
> Userspace can close the sync device while there are still active fence
> points, in which case kernel produces the following warning:
>
> [ 43.853176] [ cut here ]
> [ 43.857834] WARNING: CPU: 0 PID: 892 at
> /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
> android_fence_release+0x88/0x104()
> [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U
> 3.18.0-07661-g0550ce9 #1
> [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
> [ 43.885834] Call trace:
> [ 43.888294] [] dump_backtrace+0x0/0x10c
> [ 43.893697] [] show_stack+0x10/0x1c
> [ 43.898756] [] dump_stack+0x74/0xb8
> [ 43.903814] [] warn_slowpath_common+0x84/0xb0
> [ 43.909736] [] warn_slowpath_null+0x14/0x20
> [ 43.915482] [] android_fence_release+0x84/0x104
> [ 43.921582] [] fence_release+0x104/0x134
> [ 43.927066] [] sync_fence_free+0x74/0x9c
> [ 43.932552] [] sync_fence_release+0x34/0x48
> [ 43.938304] [] __fput+0x100/0x1b8
> [ 43.943185] [] fput+0x8/0x14
> [ 43.947982] [] task_work_run+0xb0/0xe4
> [ 43.953297] [] do_notify_resume+0x44/0x5c
> [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]---
>
> Let's fix it by introducing a new optional callback (disable_signaling)
> to fence operations so that drivers can do proper clean ups when we
> remove last callback for given fence.
>
> Reviewed-by: Andrew Bresticker
> Signed-off-by: Dmitry Torokhov
> ---
> drivers/dma-buf/fence.c| 6 +-
> drivers/staging/android/sync.c | 8
> include/linux/fence.h | 2 ++
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
> index 7b05dbe..0ed73ad 100644
> --- a/drivers/dma-buf/fence.c
> +++ b/drivers/dma-buf/fence.c
> @@ -304,8 +304,12 @@ fence_remove_callback(struct fence *fence, struct
> fence_cb *cb)
> spin_lock_irqsave(fence->lock, flags);
>
> ret = !list_empty(&cb->node);
> - if (ret)
> + if (ret) {
> list_del_init(&cb->node);
> + if (list_empty(&fence->cb_list))
> + if (fence->ops->disable_signaling)
> + fence->ops->disable_signaling(fence);
What exactly is the bug here? A fence with no callbacks registered any
more shouldn't have any problem. Why exactly does this blow up?
I guess I don't really understand the bug ... we do seem to remove the
callback already.
Thanks, Daniel
> + }
>
> spin_unlock_irqrestore(fence->lock, flags);
>
> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> index e0c1acb..f8566c1 100644
> --- a/drivers/staging/android/sync.c
> +++ b/drivers/staging/android/sync.c
> @@ -465,6 +465,13 @@ static bool android_fence_enable_signaling(struct fence
> *fence)
> return true;
> }
>
> +static void android_fence_disable_signaling(struct fence *fence)
> +{
> + struct sync_pt *pt = container_of(fence, struct sync_pt, base);
> +
> + list_del_init(&pt->active_list);
> +}
> +
> static int android_fence_fill_driver_data(struct fence *fence,
> void *data, int size)
> {
> @@ -508,6 +515,7 @@ static const struct fence_ops android_fence_ops = {
> .get_driver_name = android_fence_get_driver_name,
> .get_timeline_name = android_fence_get_timeline_name,
> .enable_signaling = android_fence_enable_signaling,
> + .disable_signaling = android_fence_disable_signaling,
> .signaled = android_fence_signaled,
> .wait = fence_default_wait,
> .release = android_fence_release,
> diff --git a/include/linux/fence.h b/include/linux/fence.h
> index bb52201..ce44348 100644
> --- a/include/linux/fence.h
> +++ b/include/linux/fence.h
> @@ -107,6 +107,7 @@ struct fence_cb {
> * @get_driver_name: returns the driver name.
> * @get_timeline_name: return the name of the context this fence belongs to.
> * @enable_signaling: enable software signaling of fence.
> + * @disable_signaling: disable software signaling of fence (optional).
> * @signaled: [optional] peek whether the fence is signaled, can be null.
> * @wait: custom wait implementation, or fence_default_wait.
> * @release: [optional] called on destruction of fence, can be null
> @@ -166,6 +167,7 @@ struct fence_ops {
> const char * (*get_driver_name)(struct fence *fence);
> const char * (*get_timeline_name)(struct fence *fence);
> bool (*enable_signaling)(struct fence *fence);
> + void (*disable_signaling)(struct fence *fence);
> bool (*signaled)(struct fence *fence);
> signed long (*wait)(struct fence *fence, bool intr, signed long
> timeout);
> void (*release)(struct fence *fence);
> --
> 2.6.0.rc2.230.g3dd15c0
>
>
> --
> Dmitry
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> http://lists
Re: [PATCH] android: fix warning when releasing active sync point
Op 15-12-15 om 02:29 schreef Dmitry Torokhov: > Userspace can close the sync device while there are still active fence > points, in which case kernel produces the following warning: > > [ 43.853176] [ cut here ] > [ 43.857834] WARNING: CPU: 0 PID: 892 at > /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439 > android_fence_release+0x88/0x104() > [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U > 3.18.0-07661-g0550ce9 #1 > [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT) > [ 43.885834] Call trace: > [ 43.888294] [] dump_backtrace+0x0/0x10c > [ 43.893697] [] show_stack+0x10/0x1c > [ 43.898756] [] dump_stack+0x74/0xb8 > [ 43.903814] [] warn_slowpath_common+0x84/0xb0 > [ 43.909736] [] warn_slowpath_null+0x14/0x20 > [ 43.915482] [] android_fence_release+0x84/0x104 > [ 43.921582] [] fence_release+0x104/0x134 > [ 43.927066] [] sync_fence_free+0x74/0x9c > [ 43.932552] [] sync_fence_release+0x34/0x48 > [ 43.938304] [] __fput+0x100/0x1b8 > [ 43.943185] [] fput+0x8/0x14 > [ 43.947982] [] task_work_run+0xb0/0xe4 > [ 43.953297] [] do_notify_resume+0x44/0x5c > [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]--- > > Let's fix it by introducing a new optional callback (disable_signaling) > to fence operations so that drivers can do proper clean ups when we > remove last callback for given fence. > > Reviewed-by: Andrew Bresticker > Signed-off-by: Dmitry Torokhov > NACK! There's no way to do this race free. The driver should hold a refcount until fence is signaled. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: lustre: Less checks in mgc_process_recover_log() after error detection
On Mon, Dec 14, 2015 at 06:43:15PM +0100, SF Markus Elfring wrote: > Our software development dialogue seems to trigger special > challenges between us so far. I try very hard to review patches mechanically and not be biased so that after a while people know if their patches will be merged or not without waiting for feedback. In this case, I had asked you not to send patches renaming out labels and then the next day you sent me a string of patches renaming out labels. If you were a lustre dev then I would accept these renames definitely. But I believe that for anyone else, I would ask them what the point of doing these renames is. I do not think I have been unfair to you. There was no element of surprise. Part of the reason we have CodingStyle is so that we can tell people "That's not in CodingStyle, that's just your own opinion so don't redo code just because you have a different opinion from the maintainer." > Are you generally willing to change the exception handling for > the memory allocations in the function "mgc_process_recover_log" > at all? I like the first patch in this series. I do not like the renames. I don't care too much about patches 5 and 6 except that they should be folded together and you should not move "req" and "eof" around. Mostly I wish you would just focus on fixing bugs instead of these sorts of patches. It is a lot of work for me to explain how to redo patches but it is worth it for bugfixes. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 3/9] Drivers: hv: utils: introduce HVUTIL_TRANSPORT_DESTROY mode
> -Original Message- > From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf > Of K. Y. Srinivasan > Sent: Tuesday, December 15, 2015 11:02 > To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; > vkuzn...@redhat.com; jasow...@redhat.com > Subject: [PATCH 3/9] Drivers: hv: utils: introduce HVUTIL_TRANSPORT_DESTROY > mode > > From: Vitaly Kuznetsov > > When Hyper-V host asks us to remove some util driver by closing the > appropriate channel there is no easy way to force the current file > descriptor holder to hang up but we can start to respond -EBADF to all > operations asking it to exit gracefully. > > As we're setting hvt->mode from two separate contexts now we need to use > a proper locking. > > ... > @@ -99,6 +107,10 @@ static unsigned int hvt_op_poll(struct file *file, > poll_table *wait) > hvt = container_of(file->f_op, struct hvutil_transport, fops); > > poll_wait(file, &hvt->outmsg_q, wait); > + > + if (hvt->mode == HVUTIL_TRANSPORT_DESTROY) > + return -EBADF; > + > if (hvt->outmsg_len > 0) > return POLLIN | POLLRDNORM; Hi Vitaly, Should hvt_op_poll() return -EBADF -- I think it probably should return POLLERR or POLLHUP? -- Dexuan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: fix warning when releasing active sync point
2015-12-14 Dmitry Torokhov : > Userspace can close the sync device while there are still active fence > points, in which case kernel produces the following warning: > > [ 43.853176] [ cut here ] > [ 43.857834] WARNING: CPU: 0 PID: 892 at > /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439 > android_fence_release+0x88/0x104() > [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U > 3.18.0-07661-g0550ce9 #1 > [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT) > [ 43.885834] Call trace: > [ 43.888294] [] dump_backtrace+0x0/0x10c > [ 43.893697] [] show_stack+0x10/0x1c > [ 43.898756] [] dump_stack+0x74/0xb8 > [ 43.903814] [] warn_slowpath_common+0x84/0xb0 > [ 43.909736] [] warn_slowpath_null+0x14/0x20 > [ 43.915482] [] android_fence_release+0x84/0x104 > [ 43.921582] [] fence_release+0x104/0x134 > [ 43.927066] [] sync_fence_free+0x74/0x9c > [ 43.932552] [] sync_fence_release+0x34/0x48 > [ 43.938304] [] __fput+0x100/0x1b8 > [ 43.943185] [] fput+0x8/0x14 > [ 43.947982] [] task_work_run+0xb0/0xe4 > [ 43.953297] [] do_notify_resume+0x44/0x5c > [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]--- This crash report seems to be for a 3.18 kernel. Can you reproduce it on upstream kernel as well? Gustavo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: fix warning when releasing active sync point
Is this not the issue fixed by 8e43c9c75? Thanks Frank On 15/12/15 13:30, Gustavo Padovan wrote: > 2015-12-14 Dmitry Torokhov : > >> Userspace can close the sync device while there are still active fence >> points, in which case kernel produces the following warning: >> >> [ 43.853176] [ cut here ] >> [ 43.857834] WARNING: CPU: 0 PID: 892 at >> /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439 >> android_fence_release+0x88/0x104() >> [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U >> 3.18.0-07661-g0550ce9 #1 >> [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT) >> [ 43.885834] Call trace: >> [ 43.888294] [] dump_backtrace+0x0/0x10c >> [ 43.893697] [] show_stack+0x10/0x1c >> [ 43.898756] [] dump_stack+0x74/0xb8 >> [ 43.903814] [] warn_slowpath_common+0x84/0xb0 >> [ 43.909736] [] warn_slowpath_null+0x14/0x20 >> [ 43.915482] [] android_fence_release+0x84/0x104 >> [ 43.921582] [] fence_release+0x104/0x134 >> [ 43.927066] [] sync_fence_free+0x74/0x9c >> [ 43.932552] [] sync_fence_release+0x34/0x48 >> [ 43.938304] [] __fput+0x100/0x1b8 >> [ 43.943185] [] fput+0x8/0x14 >> [ 43.947982] [] task_work_run+0xb0/0xe4 >> [ 43.953297] [] do_notify_resume+0x44/0x5c >> [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]--- > This crash report seems to be for a 3.18 kernel. Can you reproduce it > on upstream kernel as well? > > Gustavo > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions
On Tue, Dec 15, 2015 at 06:27:56AM -0800, Joe Perches wrote: > On Sun, 2015-12-13 at 14:52 +0100, SF Markus Elfring wrote: > > From: Markus Elfring > > Date: Sun, 13 Dec 2015 09:30:47 +0100 > > > > Six goto statements referred to a source code position directly behind them. > > Thus omit such unnecessary jumps. > > I suggest you leave a blank line instead > of deleting the goto. > What is the point of the little bunny hop? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: Skein: Moved macros from skein_block.c to header file.
On 14 December 2015 at 17:08, Sanidhya Solanki wrote:
> The original code defined macros in the source code, making it
> harder to read. Moved them to the header file, as per the TODO file.
>
> Updated the TODO file.
>
> Signed-off-by: Sanidhya Solanki
> ---
> drivers/staging/skein/TODO | 1 -
> drivers/staging/skein/skein_block.c | 6 --
> drivers/staging/skein/skein_block.h | 7 +++
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/skein/TODO b/drivers/staging/skein/TODO
> index cd3508d..e3de0c7 100644
> --- a/drivers/staging/skein/TODO
> +++ b/drivers/staging/skein/TODO
> @@ -1,6 +1,5 @@
> skein/threefish TODO
>
> - - move macros into appropriate header files
> - add / pass test vectors
> - module support
>
> diff --git a/drivers/staging/skein/skein_block.c
> b/drivers/staging/skein/skein_block.c
> index 45b4732..2120392 100644
> --- a/drivers/staging/skein/skein_block.c
> +++ b/drivers/staging/skein/skein_block.c
> @@ -26,12 +26,6 @@
> #define SKEIN_LOOP 001 /* default: unroll 256 and 512, but not 1024 */
> #endif
>
> -#define BLK_BITS(WCNT * 64) /* some useful definitions for code here
> */
> -#define KW_TWK_BASE (0)
> -#define KW_KEY_BASE (3)
> -#define ks (kw + KW_KEY_BASE)
> -#define ts (kw + KW_TWK_BASE)
> -
> #ifdef SKEIN_DEBUG
> #define debug_save_tweak(ctx) \
> { \
> diff --git a/drivers/staging/skein/skein_block.h
> b/drivers/staging/skein/skein_block.h
> index 9d40f4a..0fd4bfe 100644
> --- a/drivers/staging/skein/skein_block.h
> +++ b/drivers/staging/skein/skein_block.h
> @@ -7,6 +7,13 @@
> ** This algorithm and source code is released to the public domain.
> **
> /
> +
> +#define BLK_BITS(WCNT * 64) /* some useful definitions for code here
> */
> +#define KW_TWK_BASE (0)
> +#define KW_KEY_BASE (3)
> +#define ks (kw + KW_KEY_BASE)
> +#define ts (kw + KW_TWK_BASE)
> +
> #ifndef _SKEIN_BLOCK_H_
> #define _SKEIN_BLOCK_H_
>
> --
> 2.5.0
>
I must admit you lost me here - what is this new version about? I
suggest you used the [PATCH v#] convention along with a log of
modifications from one version to another when sending new revisions.
That way people know what to look for.
Thanks,
Mathieu
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: lustre: Less checks in mgc_process_recover_log() after error detection
> If you were a lustre dev then I would accept these renames definitely. I find this information interesting. Would any more contributors like to share their opinion? > I do not think I have been unfair to you. This view is correct in principle. > There was no element of surprise. I am trying to discuss further "special" update suggestions where the topic focus might evolve to new directions. I got the impression that you had some difficulties already with my previous proposals. So I am unsure about the general change acceptance from you alone. You pointed out that you are maintainer for this software area. I was not so aware about this detail while I noticed that you are very active Linux software developer. (You are not mentioned in the file "MAINTAINERS" for example.) > Part of the reason we have CodingStyle is so that we can tell people > "That's not in CodingStyle, that's just your own opinion so don't redo > code just because you have a different opinion from the maintainer." I find this description reasonable. But I see some challenges to improve the coding style specification. I would appreciate if some items can become less ambiguous and imprecise. I assume that a few recommendations from the script "checkpatch.pl" should also be mentioned there. > >> Are you generally willing to change the exception handling for >> the memory allocations in the function "mgc_process_recover_log" >> at all? > I like the first patch in this series. Thanks for a bit of positive feedback. > I do not like the renames. I guess that this design aspect can be clarified a bit more. > I don't care too much about patches 5 and 6 except that they should be > folded together and you should not move "req" and "eof" around. I can understand this concern better than your first response for these update steps. I might send an adjusted patch series a few days later. > Mostly I wish you would just focus on fixing bugs instead of these sorts > of patches. How often are deviations from the coding style also just ordinary bugs? It seems that changes for this area are occasionally not so attractive in comparison to software improvements for components which are more popular. Regards, Markus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions
On Sun, 2015-12-13 at 14:52 +0100, SF Markus Elfring wrote: > From: Markus Elfring > Date: Sun, 13 Dec 2015 09:30:47 +0100 > > Six goto statements referred to a source code position directly behind them. > Thus omit such unnecessary jumps. I suggest you leave a blank line instead of deleting the goto. > diff --git a/drivers/staging/lustre/lustre/llite/namei.c > b/drivers/staging/lustre/lustre/llite/namei.c [] > @@ -554,7 +554,6 @@ static struct dentry *ll_lookup_it(struct inode *parent, > struct dentry *dentry, > retval = NULL; > else > retval = dentry; > - goto out; > out: > if (req) > ptlrpc_req_finished(req); etc.,, ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: wlan-ng: use list_for_each_entry*
Use list_for_each_entry*() instead of list_for_each*() to simplify
the code.
Signed-off-by: Geliang Tang
---
drivers/staging/wlan-ng/hfa384x_usb.c | 17 -
drivers/staging/wlan-ng/prism2usb.c | 15 +++
2 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c
b/drivers/staging/wlan-ng/hfa384x_usb.c
index 7551ac2..9b04036 100644
--- a/drivers/staging/wlan-ng/hfa384x_usb.c
+++ b/drivers/staging/wlan-ng/hfa384x_usb.c
@@ -2810,8 +2810,7 @@ void hfa384x_tx_timeout(wlandevice_t *wlandev)
static void hfa384x_usbctlx_reaper_task(unsigned long data)
{
hfa384x_t *hw = (hfa384x_t *)data;
- struct list_head *entry;
- struct list_head *temp;
+ hfa384x_usbctlx_t *ctlx, *temp;
unsigned long flags;
spin_lock_irqsave(&hw->ctlxq.lock, flags);
@@ -2819,10 +2818,7 @@ static void hfa384x_usbctlx_reaper_task(unsigned long
data)
/* This list is guaranteed to be empty if someone
* has unplugged the adapter.
*/
- list_for_each_safe(entry, temp, &hw->ctlxq.reapable) {
- hfa384x_usbctlx_t *ctlx;
-
- ctlx = list_entry(entry, hfa384x_usbctlx_t, list);
+ list_for_each_entry_safe(ctlx, temp, &hw->ctlxq.reapable, list) {
list_del(&ctlx->list);
kfree(ctlx);
}
@@ -2847,8 +2843,7 @@ static void hfa384x_usbctlx_reaper_task(unsigned long
data)
static void hfa384x_usbctlx_completion_task(unsigned long data)
{
hfa384x_t *hw = (hfa384x_t *)data;
- struct list_head *entry;
- struct list_head *temp;
+ hfa384x_usbctlx_t *ctlx, *temp;
unsigned long flags;
int reap = 0;
@@ -2858,11 +2853,7 @@ static void hfa384x_usbctlx_completion_task(unsigned
long data)
/* This list is guaranteed to be empty if someone
* has unplugged the adapter ...
*/
- list_for_each_safe(entry, temp, &hw->ctlxq.completing) {
- hfa384x_usbctlx_t *ctlx;
-
- ctlx = list_entry(entry, hfa384x_usbctlx_t, list);
-
+ list_for_each_entry_safe(ctlx, temp, &hw->ctlxq.completing, list) {
/* Call the completion function that this
* command was assigned, assuming it has one.
*/
diff --git a/drivers/staging/wlan-ng/prism2usb.c
b/drivers/staging/wlan-ng/prism2usb.c
index 8abf3f8..194f67e 100644
--- a/drivers/staging/wlan-ng/prism2usb.c
+++ b/drivers/staging/wlan-ng/prism2usb.c
@@ -139,8 +139,7 @@ static void prism2sta_disconnect_usb(struct usb_interface
*interface)
wlandev = (wlandevice_t *)usb_get_intfdata(interface);
if (wlandev != NULL) {
LIST_HEAD(cleanlist);
- struct list_head *entry;
- struct list_head *temp;
+ hfa384x_usbctlx_t *ctlx, *temp;
unsigned long flags;
hfa384x_t *hw = wlandev->priv;
@@ -184,12 +183,8 @@ static void prism2sta_disconnect_usb(struct usb_interface
*interface)
* and tell everyone who is waiting for their
* responses that we have shut down.
*/
- list_for_each(entry, &cleanlist) {
- hfa384x_usbctlx_t *ctlx;
-
- ctlx = list_entry(entry, hfa384x_usbctlx_t, list);
+ list_for_each_entry(ctlx, &cleanlist, list)
complete(&ctlx->done);
- }
/* Give any outstanding synchronous commands
* a chance to complete. All they need to do
@@ -199,12 +194,8 @@ static void prism2sta_disconnect_usb(struct usb_interface
*interface)
msleep(100);
/* Now delete the CTLXs, because no-one else can now. */
- list_for_each_safe(entry, temp, &cleanlist) {
- hfa384x_usbctlx_t *ctlx;
-
- ctlx = list_entry(entry, hfa384x_usbctlx_t, list);
+ list_for_each_entry_safe(ctlx, temp, &cleanlist, list)
kfree(ctlx);
- }
/* Unhook the wlandev */
unregister_wlandev(wlandev);
--
2.5.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions
On Tue, 2015-12-15 at 17:41 +0300, Dan Carpenter wrote: > On Tue, Dec 15, 2015 at 06:27:56AM -0800, Joe Perches wrote: > > On Sun, 2015-12-13 at 14:52 +0100, SF Markus Elfring wrote: > > > From: Markus Elfring > > > Date: Sun, 13 Dec 2015 09:30:47 +0100 > > > > > > Six goto statements referred to a source code position directly behind > > > them. > > > Thus omit such unnecessary jumps. > > > > I suggest you leave a blank line instead > > of deleting the goto. > > > > What is the point of the little bunny hop? > > regards, > dan carpenter > -ENOPARSE little bunny hop (though I could have said "just leave a blank line) I think that code blocks are more obvious to read. This is the original code: result = foo(); if (result) goto label; result = bar(); if (result) goto label; result = baz(); if (result) goto label; label: go on... He proposes: result = foo(); if (result) goto label; result = bar(); if (result) goto label; result = baz(); label: go on... I don't find the test->goto label; label: use offensive, but if he does, I think keeping a blank line in place of the test->goto might be better. result = foo(); if (result) goto label; result = bar(); if (result) goto label; result = baz(); label: go on... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/9] Drivers: hv: utils: introduce HVUTIL_TRANSPORT_DESTROY mode
Dexuan Cui writes: >> -Original Message- >> From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf >> Of K. Y. Srinivasan >> Sent: Tuesday, December 15, 2015 11:02 >> To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; >> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; >> vkuzn...@redhat.com; jasow...@redhat.com >> Subject: [PATCH 3/9] Drivers: hv: utils: introduce HVUTIL_TRANSPORT_DESTROY >> mode >> >> From: Vitaly Kuznetsov >> >> When Hyper-V host asks us to remove some util driver by closing the >> appropriate channel there is no easy way to force the current file >> descriptor holder to hang up but we can start to respond -EBADF to all >> operations asking it to exit gracefully. >> >> As we're setting hvt->mode from two separate contexts now we need to use >> a proper locking. >> >> ... >> @@ -99,6 +107,10 @@ static unsigned int hvt_op_poll(struct file *file, >> poll_table *wait) >> hvt = container_of(file->f_op, struct hvutil_transport, fops); >> >> poll_wait(file, &hvt->outmsg_q, wait); >> + >> +if (hvt->mode == HVUTIL_TRANSPORT_DESTROY) >> +return -EBADF; >> + >> if (hvt->outmsg_len > 0) >> return POLLIN | POLLRDNORM; > > Hi Vitaly, > Should hvt_op_poll() return -EBADF -- I think it probably > should return POLLERR or POLLHUP? Oh, sorry, my bad -- hvt_op_poll() returns unsigned int and -EBADF is definitely inappropriate. I see this patch was already merged to char-misc-testing so I'll send a follow-up patch to fix things up. Thanks! -- Vitaly ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: fix warning when releasing active sync point
On Tue, Dec 15, 2015 at 5:50 AM, Frank Binns wrote: > Is this not the issue fixed by 8e43c9c75? No because if we start teardown without waiting for the fence to be signaled it will still be on the active_list. Thanks. -- Dmitry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: fix warning when releasing active sync point
On Tue, Dec 15, 2015 at 2:01 AM, Maarten Lankhorst wrote: > Op 15-12-15 om 02:29 schreef Dmitry Torokhov: >> Userspace can close the sync device while there are still active fence >> points, in which case kernel produces the following warning: >> >> [ 43.853176] [ cut here ] >> [ 43.857834] WARNING: CPU: 0 PID: 892 at >> /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439 >> android_fence_release+0x88/0x104() >> [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U >> 3.18.0-07661-g0550ce9 #1 >> [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT) >> [ 43.885834] Call trace: >> [ 43.888294] [] dump_backtrace+0x0/0x10c >> [ 43.893697] [] show_stack+0x10/0x1c >> [ 43.898756] [] dump_stack+0x74/0xb8 >> [ 43.903814] [] warn_slowpath_common+0x84/0xb0 >> [ 43.909736] [] warn_slowpath_null+0x14/0x20 >> [ 43.915482] [] android_fence_release+0x84/0x104 >> [ 43.921582] [] fence_release+0x104/0x134 >> [ 43.927066] [] sync_fence_free+0x74/0x9c >> [ 43.932552] [] sync_fence_release+0x34/0x48 >> [ 43.938304] [] __fput+0x100/0x1b8 >> [ 43.943185] [] fput+0x8/0x14 >> [ 43.947982] [] task_work_run+0xb0/0xe4 >> [ 43.953297] [] do_notify_resume+0x44/0x5c >> [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]--- >> >> Let's fix it by introducing a new optional callback (disable_signaling) >> to fence operations so that drivers can do proper clean ups when we >> remove last callback for given fence. >> >> Reviewed-by: Andrew Bresticker >> Signed-off-by: Dmitry Torokhov >> > NACK! There's no way to do this race free. Can you please explain the race because as far as I can see there is not one. > The driver should hold a refcount until fence is signaled. If we are no longer interested in fence why do we need to wait for the fence to be signaled? Thanks. -- Dmitry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 21/40] staging: lustre: improve LNet clean up code and API
>Actually we're going to have to redo so much code that it's not worth it >for me to review the rest of these patches. Sorry I didn't get back to you sooner but I was on vacation. Thanks for reviewing this work. Especially since this is the first major bug fixing merge for the lustre client which means a lot of pain involved to iron out how to do this. I have been pondering if pushing bug fixes before style cleanups is the right thing to do. I pushed a bunch of bug fixes earlier and none got merged which either means Greg is just backed up and hasn't the time to merge them or style issues are higher priority. Assuming these bug fixes are in scope of the staging tree. Should I continue to push this work first? Well either way I should update this patch series so it ready to merge at some point. >Please just look over everything again: > > BAD: return -1; >GOOD: return -EINVAL; > > BAD: failed0: >GOOD: free_something: > > BAD: if (rc != 0) >GOOD: if (rc) > >Do one thing per patch. >Do not introduce a bug and then fix it in a later patch. >Check ioc_len more carefully. >Don't make the code look ugly just to please checkpatch.pl. >Do error handling not success handling. >Try to avoid indenting a far to the right. Okay. Will start to do the patch cleanup. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: fix warning when releasing active sync point
On Tue, Dec 15, 2015 at 5:30 AM, Gustavo Padovan wrote: > 2015-12-14 Dmitry Torokhov : > >> Userspace can close the sync device while there are still active fence >> points, in which case kernel produces the following warning: >> >> [ 43.853176] [ cut here ] >> [ 43.857834] WARNING: CPU: 0 PID: 892 at >> /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439 >> android_fence_release+0x88/0x104() >> [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U >> 3.18.0-07661-g0550ce9 #1 >> [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT) >> [ 43.885834] Call trace: >> [ 43.888294] [] dump_backtrace+0x0/0x10c >> [ 43.893697] [] show_stack+0x10/0x1c >> [ 43.898756] [] dump_stack+0x74/0xb8 >> [ 43.903814] [] warn_slowpath_common+0x84/0xb0 >> [ 43.909736] [] warn_slowpath_null+0x14/0x20 >> [ 43.915482] [] android_fence_release+0x84/0x104 >> [ 43.921582] [] fence_release+0x104/0x134 >> [ 43.927066] [] sync_fence_free+0x74/0x9c >> [ 43.932552] [] sync_fence_release+0x34/0x48 >> [ 43.938304] [] __fput+0x100/0x1b8 >> [ 43.943185] [] fput+0x8/0x14 >> [ 43.947982] [] task_work_run+0xb0/0xe4 >> [ 43.953297] [] do_notify_resume+0x44/0x5c >> [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]--- > > This crash report seems to be for a 3.18 kernel. Can you reproduce it > on upstream kernel as well? Unfortunately this board does not run upsrteam just yet, but looking at the sync driver and fence code we are pretty much in sync with upstream. Thanks. -- Dmitry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v9 0/7] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs
> -Original Message- > From: ja...@microsoft.com [mailto:ja...@microsoft.com] > Sent: Wednesday, December 9, 2015 2:55 PM > To: gre...@linuxfoundation.org; KY Srinivasan ; linux- > ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de; > a...@canonical.com; vkuzn...@redhat.com; t...@linutronix.de; Haiyang > Zhang ; marc.zyng...@arm.com; > bhelg...@google.com; linux-...@vger.kernel.org > Cc: Jake Oshins > Subject: [PATCH v9 0/7] PCI: hv: New paravirtual PCI front-end for Hyper-V > VMs > > From: Jake Oshins > > This version of the patch series removes warning when compiling x86 > 32-bit. > > First, export functions that allow correlating Hyper-V virtual processors > and Linux cpus, along with the means for invoking a hypercall that targets > interrupts at chosen vectors on specific cpus. > > Second, mark various parts of IRQ domain related code as exported, so that > this PCI front-end can implement an IRQ domain as part of a module. (The > alternative would be to pull all tyhis into the kernel, which would pull > in a lot of other Hyper-V related code, as this IRQ domain depends on > vmbus.ko.) > > Third, modify PCI so that new root PCI buses can be marked with an > associated > fwnode_handle, and so that root PCI buses can look up their associated IRQ > domain by that handle. > > Fourth, introduce a new driver, hv_pcifront, which exposes root PCI buses in > a Hyper-V VM. These root PCI buses expose real PCIe device, or PCI Virtual > Functions. > > > Jake Oshins (7): > drivers:hv: Export a function that maps Linux CPU num onto Hyper-V > proc num > drivers:hv: Export hv_do_hypercall() > PCI: Make it possible to implement a PCI MSI IRQ Domain in a module. > PCI: Add fwnode_handle to pci_sysdata > PCI: irqdomain: Look up IRQ domain by fwnode_handle > drivers:hv: Define the channel type of Hyper-V PCI Express > pass-through > PCI: hv: New paravirtual PCI front-end for Hyper-V VMs > > MAINTAINERS|1 + > arch/x86/include/asm/msi.h |6 + > arch/x86/include/asm/pci.h | 15 + > arch/x86/kernel/apic/msi.c |8 +- > arch/x86/kernel/apic/vector.c |2 + > drivers/hv/hv.c| 20 +- > drivers/hv/hyperv_vmbus.h |2 +- > drivers/hv/vmbus_drv.c | 17 + > drivers/pci/Kconfig|7 + > drivers/pci/host/Makefile |1 + > drivers/pci/host/hv_pcifront.c | 2250 > > drivers/pci/msi.c |4 + > drivers/pci/probe.c| 15 + > include/linux/hyperv.h | 14 + > include/linux/pci.h|4 + > kernel/irq/chip.c |1 + > kernel/irq/irqdomain.c |4 + > 17 files changed, 2357 insertions(+), 14 deletions(-) > create mode 100644 drivers/pci/host/hv_pcifront.c > > -- > 1.9.1 Of these 7 patches, Greg has committed all of the VMBUS related supporting patches (3 patches). Thomas, can you take the IRQ related patches through your tree. Regards, K. Y ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Drivers: hv: utils: fix hvt_op_poll() return value on transport destroy
The return type of hvt_op_poll() is unsigned int and -EBADF is inappropriate, poll functions return POLL* statuses. Reported-by: Dexuan Cui Signed-off-by: Vitaly Kuznetsov --- - This is a follow-up to the previously sent 'Drivers: hv: utils: introduce HVUTIL_TRANSPORT_DESTROY mode' patch which is already in char-misc-testing tree (a15025660d47). --- drivers/hv/hv_utils_transport.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c index ee20b50..4f42c0e 100644 --- a/drivers/hv/hv_utils_transport.c +++ b/drivers/hv/hv_utils_transport.c @@ -109,7 +109,7 @@ static unsigned int hvt_op_poll(struct file *file, poll_table *wait) poll_wait(file, &hvt->outmsg_q, wait); if (hvt->mode == HVUTIL_TRANSPORT_DESTROY) - return -EBADF; + return POLLERR | POLLHUP; if (hvt->outmsg_len > 0) return POLLIN | POLLRDNORM; -- 2.4.3 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: fix warning when releasing active sync point
On Tue, Dec 15, 2015 at 1:26 AM, Daniel Vetter wrote:
> On Mon, Dec 14, 2015 at 05:29:55PM -0800, Dmitry Torokhov wrote:
>> Userspace can close the sync device while there are still active fence
>> points, in which case kernel produces the following warning:
>>
>> [ 43.853176] [ cut here ]
>> [ 43.857834] WARNING: CPU: 0 PID: 892 at
>> /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
>> android_fence_release+0x88/0x104()
>> [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U
>> 3.18.0-07661-g0550ce9 #1
>> [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
>> [ 43.885834] Call trace:
>> [ 43.888294] [] dump_backtrace+0x0/0x10c
>> [ 43.893697] [] show_stack+0x10/0x1c
>> [ 43.898756] [] dump_stack+0x74/0xb8
>> [ 43.903814] [] warn_slowpath_common+0x84/0xb0
>> [ 43.909736] [] warn_slowpath_null+0x14/0x20
>> [ 43.915482] [] android_fence_release+0x84/0x104
>> [ 43.921582] [] fence_release+0x104/0x134
>> [ 43.927066] [] sync_fence_free+0x74/0x9c
>> [ 43.932552] [] sync_fence_release+0x34/0x48
>> [ 43.938304] [] __fput+0x100/0x1b8
>> [ 43.943185] [] fput+0x8/0x14
>> [ 43.947982] [] task_work_run+0xb0/0xe4
>> [ 43.953297] [] do_notify_resume+0x44/0x5c
>> [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]---
>>
>> Let's fix it by introducing a new optional callback (disable_signaling)
>> to fence operations so that drivers can do proper clean ups when we
>> remove last callback for given fence.
>>
>> Reviewed-by: Andrew Bresticker
>> Signed-off-by: Dmitry Torokhov
>> ---
>> drivers/dma-buf/fence.c| 6 +-
>> drivers/staging/android/sync.c | 8
>> include/linux/fence.h | 2 ++
>> 3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
>> index 7b05dbe..0ed73ad 100644
>> --- a/drivers/dma-buf/fence.c
>> +++ b/drivers/dma-buf/fence.c
>> @@ -304,8 +304,12 @@ fence_remove_callback(struct fence *fence, struct
>> fence_cb *cb)
>> spin_lock_irqsave(fence->lock, flags);
>>
>> ret = !list_empty(&cb->node);
>> - if (ret)
>> + if (ret) {
>> list_del_init(&cb->node);
>> + if (list_empty(&fence->cb_list))
>> + if (fence->ops->disable_signaling)
>> + fence->ops->disable_signaling(fence);
>
> What exactly is the bug here? A fence with no callbacks registered any
> more shouldn't have any problem. Why exactly does this blow up?
>
> I guess I don't really understand the bug ... we do seem to remove the
> callback already.
>
The issue is that when enabling signalling in sync driver we put fence
on an internal list in the driver and there is no way of taking it off
this list, except when it is signalled. The driver, when destroying
the fence, checks if the fence is not on this list (as a sanity
measure) and that produces the backtrace in the commit log.
IOW for some drivers we need an "undo" for enable_signaling() callback
so that drivers can maintain consistent internal state.
Thanks.
--
Dmitry
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: lustre: Delete unnecessary goto statements in six functions
> This is the original code: Really …? > result = baz(); > if (result) > goto label; > > label: > go on... I do not see such a source code structure at the six places I propose to clean-up. > I don't find the test->goto label; label: use offensive, > but if he does, I think keeping a blank line in place of > the test->goto might be better. I find this an interesting view on source code layout. Are there any more opinions around such implementation details? Regards, Markus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 02/40] staging: lustre: fix 'NULL pointer dereference' errors for LNet
>> diff --git a/drivers/staging/lustre/lnet/selftest/conctl.c
>> b/drivers/staging/lustre/lnet/selftest/conctl.c
>> index 556c837..2ca7d0e 100644
>> --- a/drivers/staging/lustre/lnet/selftest/conctl.c
>> +++ b/drivers/staging/lustre/lnet/selftest/conctl.c
>> @@ -679,45 +679,46 @@ static int
>> lst_stat_query_ioctl(lstio_stat_args_t *args)
>> {
>> int rc;
>> -char *name;
>> +char *name = NULL;
>>
>> /* TODO: not finished */
>> if (args->lstio_sta_key != console_session.ses_key)
>> return -EACCES;
>>
>> -if (args->lstio_sta_resultp == NULL ||
>> -(args->lstio_sta_namep == NULL &&
>> - args->lstio_sta_idsp == NULL) ||
>> -args->lstio_sta_nmlen <= 0 ||
>> -args->lstio_sta_nmlen > LST_NAME_SIZE)
>> -return -EINVAL;
>> -
>> -if (args->lstio_sta_idsp != NULL &&
>> -args->lstio_sta_count <= 0)
>> +if (!args->lstio_sta_resultp)
>> return -EINVAL;
>>
>> -LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
>> -if (name == NULL)
>> -return -ENOMEM;
>> -
>> -if (copy_from_user(name, args->lstio_sta_namep,
>> - args->lstio_sta_nmlen)) {
>> -LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>> -return -EFAULT;
>> -}
>> +if (args->lstio_sta_idsp) {
>> +if (args->lstio_sta_count <= 0)
>> +return -EINVAL;
>>
>> -if (args->lstio_sta_idsp == NULL) {
>> -rc = lstcon_group_stat(name, args->lstio_sta_timeout,
>> - args->lstio_sta_resultp);
>> -} else {
>> rc = lstcon_nodes_stat(args->lstio_sta_count,
>> args->lstio_sta_idsp,
>> args->lstio_sta_timeout,
>> args->lstio_sta_resultp);
>> -}
>> +} else if (args->lstio_sta_namep) {
>> +if (args->lstio_sta_nmlen <= 0 ||
>> +args->lstio_sta_nmlen > LST_NAME_SIZE)
>> +return -EINVAL;
>> +
>> +LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
>> +if (!name)
>> +return -ENOMEM;
>>
>> -LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>> +rc = copy_from_user(name, args->lstio_sta_namep,
>> +args->lstio_sta_nmlen);
>> +if (!rc)
>> +rc = lstcon_group_stat(name, args->lstio_sta_timeout,
>> + args->lstio_sta_resultp);
>> +else
>> +rc = -EFAULT;
>>
>> +} else {
>> +rc = -EINVAL;
>> +}
>> +
>> +if (name)
>> +LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>
>There is no bug fix here. This code was fine when it was merged into
>the kernel in 2013 so I have no idea how out of date the static checker
>warning is... The new code doesn't do unnecessary allocations so that's
>good but "name" should be declared in the block where it is used instead
>of at the start of the function. Btw, we assume that the user gives us
>a NUL terminated string for "name" so we should fix that bug as well.
>
>TODO: lustre: don't assume "name" is NUL terminated
Ugh. I see breakage everywhere in this code :-( Need to address. I think we
should convert that to strcpy_to_user as well.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: fix address space mismatches
This patch fixes address space warnings from sparse. Function
lprocfs_write_helper() accepts user space buffer but was being
passed kernel space buffer by these functions:
contention_seconds_store()
lockless_truncate_store()
Since these functions are used to implement show and store functions of
lustre_attr object and since lustre_attr object is used to implement object
inheritance through use of `container_of`, the address space warnings
show up at multiple places inside driver's code base.
This patch creates a user space version of lustre_attr object lustre_attr_u.
Keeping function names and signatures same - other than the __user attribute -
ensures that object inheritance continues to work as it was, but address
space discrepency is removed. That removes a whole bunch of address
space warnings.
Signed-off-by: Okash Khawaja
---
drivers/staging/lustre/lustre/include/lprocfs_status.h | 16
drivers/staging/lustre/lustre/osc/lproc_osc.c | 12 ++--
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/lustre/lustre/include/lprocfs_status.h
b/drivers/staging/lustre/lustre/include/lprocfs_status.h
index f18c0c7..df6d9d5 100644
--- a/drivers/staging/lustre/lustre/include/lprocfs_status.h
+++ b/drivers/staging/lustre/lustre/include/lprocfs_status.h
@@ -698,6 +698,22 @@ static struct lustre_attr lustre_attr_##name =
__ATTR(name, mode, show, store)
#define LUSTRE_RO_ATTR(name) LUSTRE_ATTR(name, 0444, name##_show, NULL)
#define LUSTRE_RW_ATTR(name) LUSTRE_ATTR(name, 0644, name##_show, name##_store)
+struct lustre_attr_u {
+ struct attribute attr;
+ ssize_t (*show)(struct kobject *kobj, struct attribute *attr,
+ char *buf);
+ ssize_t (*store)(struct kobject *kobj, struct attribute *attr,
+const char __user *buf, size_t len);
+};
+
+#define LUSTRE_ATTR_U(name, mode, show, store) \
+static struct lustre_attr_u lustre_attr_u_##name = __ATTR(name, mode, show, \
+ store)
+
+#define LUSTRE_RO_ATTR_U(name) LUSTRE_ATTR_U(name, 0444, name##_show, NULL)
+#define LUSTRE_RW_ATTR_U(name) LUSTRE_ATTR_U(name, 0644, name##_show, \
+ name##_store)
+
extern const struct sysfs_ops lustre_sysfs_ops;
/* all quota proc functions */
diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c
b/drivers/staging/lustre/lustre/osc/lproc_osc.c
index c4d44e7..dd80780 100644
--- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
+++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
@@ -474,7 +474,7 @@ static ssize_t contention_seconds_show(struct kobject *kobj,
static ssize_t contention_seconds_store(struct kobject *kobj,
struct attribute *attr,
- const char *buffer,
+ const char __user *buffer,
size_t count)
{
struct obd_device *obd = container_of(kobj, struct obd_device,
@@ -484,7 +484,7 @@ static ssize_t contention_seconds_store(struct kobject
*kobj,
return lprocfs_write_helper(buffer, count, &od->od_contention_time) ?:
count;
}
-LUSTRE_RW_ATTR(contention_seconds);
+LUSTRE_RW_ATTR_U(contention_seconds);
static ssize_t lockless_truncate_show(struct kobject *kobj,
struct attribute *attr,
@@ -499,7 +499,7 @@ static ssize_t lockless_truncate_show(struct kobject *kobj,
static ssize_t lockless_truncate_store(struct kobject *kobj,
struct attribute *attr,
- const char *buffer,
+ const char __user *buffer,
size_t count)
{
struct obd_device *obd = container_of(kobj, struct obd_device,
@@ -509,7 +509,7 @@ static ssize_t lockless_truncate_store(struct kobject *kobj,
return lprocfs_write_helper(buffer, count, &od->od_lockless_truncate) ?:
count;
}
-LUSTRE_RW_ATTR(lockless_truncate);
+LUSTRE_RW_ATTR_U(lockless_truncate);
static ssize_t destroys_in_flight_show(struct kobject *kobj,
struct attribute *attr,
@@ -766,13 +766,13 @@ int lproc_osc_attach_seqstat(struct obd_device *dev)
static struct attribute *osc_attrs[] = {
&lustre_attr_active.attr,
&lustre_attr_checksums.attr,
- &lustre_attr_contention_seconds.attr,
+ &lustre_attr_u_contention_seconds.attr,
&lustre_attr_cur_dirty_bytes.attr,
&lustre_attr_cur_grant_bytes.attr,
&lustre_attr_cur_lost_grant_bytes.attr,
&lustre_attr_destroys_in_flight.attr,
&lustre_attr_grant_shrink_interval.attr,
- &lustre_attr_lockless_truncate.attr,
+ &lustre_attr_u_lockless_truncate.attr,
&lustre_attr_max_dirty_mb.attr,
&lustre_attr_max_pages_per_rpc.attr,
&lustre_attr_max_rpcs_in_flight.attr,
--
2.5.2
___
Re: [PATCH RESEND 22/27] tools/hv: Use include/uapi with __EXPORTED_HEADERS__
On Mon, 2015-12-14 at 19:11 -0800, Greg KH wrote:
> On Mon, Dec 14, 2015 at 04:01:53PM -0800, K. Y. Srinivasan wrote:
> > From: Kamal Mostafa
> >
> > Use the local uapi headers to keep in sync with "recently" added #define's
> > (e.g. VSS_OP_REGISTER1).
> >
> > Fixes: 3eb2094c59e89db2bedd401e23c7a870081c9edb
>
> Please use the "correct" way of listing this:
> 3eb2094c59e8 ("Adding makefile for tools/hv")
>
> And as this is an old kernel, shouldn't it go into stable releases?
Yes it should. I'll resubmit with Cc: stable and correct the Fixes
line.
> Or
> does that mean that no one really ever runs into this issue?
>
> > Signed-off-by: Kamal Mostafa
> > Signed-off-by: K. Y. Srinivasan
> > ---
> > tools/hv/Makefile |2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/tools/hv/Makefile b/tools/hv/Makefile
> > index a8ab795..a8c4644 100644
> > --- a/tools/hv/Makefile
> > +++ b/tools/hv/Makefile
> > @@ -5,6 +5,8 @@ PTHREAD_LIBS = -lpthread
> > WARNINGS = -Wall -Wextra
> > CFLAGS = $(WARNINGS) -g $(PTHREAD_LIBS) $(shell getconf LFS_CFLAGS)
> >
> > +CFLAGS += -D__EXPORTED_HEADERS__ -I../../include/uapi -I../../include
>
> Why do you need ../../include also?
Because include/uapi/linux/stddef.h includes linux/compiler.h, so
omitting ../../include yields:
gcc -Wall -Wextra -g -lpthread -D__EXPORTED_HEADERS__ -I../../include/uapi -o
hv_kvp_daemon hv_kvp_daemon.c
In file included from ../../include/uapi/linux/posix_types.h:4:0,
from ../../include/uapi/linux/types.h:13,
from ../../include/uapi/linux/uuid.h:24,
from ../../include/uapi/linux/hyperv.h:28,
from hv_kvp_daemon.c:36:
../../include/uapi/linux/stddef.h:1:28: fatal error: linux/compiler.h: No such
file or directory
#include
^
-Kamal
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] tools/hv: Use include/uapi with __EXPORTED_HEADERS__
>From f424410bf4a6a0dd6ff88bec808798016f4161a0 Mon Sep 17 00:00:00 2001
From: Kamal Mostafa
Date: Wed, 11 Nov 2015 19:16:59 +
Subject: tools/hv: Use include/uapi with __EXPORTED_HEADERS__
Use the local uapi headers to keep in sync with "recently" added #define's
(e.g. VSS_OP_REGISTER1).
Fixes: 3eb2094c59e8 ("Adding makefile for tools/hv")
Cc:
Signed-off-by: Kamal Mostafa
---
tools/hv/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/hv/Makefile b/tools/hv/Makefile
index a8ab795..a8c4644 100644
--- a/tools/hv/Makefile
+++ b/tools/hv/Makefile
@@ -5,6 +5,8 @@ PTHREAD_LIBS = -lpthread
WARNINGS = -Wall -Wextra
CFLAGS = $(WARNINGS) -g $(PTHREAD_LIBS) $(shell getconf LFS_CFLAGS)
+CFLAGS += -D__EXPORTED_HEADERS__ -I../../include/uapi -I../../include
+
all: hv_kvp_daemon hv_vss_daemon hv_fcopy_daemon
%: %.c
$(CC) $(CFLAGS) -o $@ $^
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command
On Tue, Dec 15, 2015 at 06:14:19PM +, Simmons, James A. wrote:
>
> >> struct libcfs_ioctl_hdr {
> >>__u32 ioc_len;
> >> @@ -87,6 +88,13 @@ do {\
> >>data.ioc_hdr.ioc_len = sizeof(data);\
> >> } while (0)
> >>
> >> +#define LIBCFS_IOC_INIT_V2(data, hdr) \
> >> +do { \
> >> + memset(&(data), 0, sizeof(data)); \
> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
> >> + (data).hdr.ioc_len = sizeof(data); \
> >> +} while (0)
> >> +
> >
> >Do we really need this?
>
> Would you be okay if this was a inline function? This is used by user land
> and kernel space code.
>
I try (not very hard) to sound like a broken record but this business of
sharing code with userland is a pain in the butt. It's not used in the
kernel or in any patches you have sent.
It would look better as an inline function though so I wouldn't have
even noticed it.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: lustre: Delete unnecessary goto statements in six functions
> rc = mdc_queue_wait(req); > goto out; > out: > ptlrpc_req_finished(req); > return rc; > } > - > > I think if the last goto out; is to be removed, > then it should be replaced by a blank line. > > It separates the last operation block from the return. Would you like to point a very specific coding style issue out? How often should jump labels preceded with blank lines? Regards, Markus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command
>On Tue, Dec 15, 2015 at 06:14:19PM +, Simmons, James A. wrote:
>>
>> >> struct libcfs_ioctl_hdr {
>> >> __u32 ioc_len;
>> >> @@ -87,6 +88,13 @@ do { \
>> >> data.ioc_hdr.ioc_len = sizeof(data);\
>> >> } while (0)
>> >>
>> >> +#define LIBCFS_IOC_INIT_V2(data, hdr)\
>> >> +do { \
>> >> + memset(&(data), 0, sizeof(data)); \
>> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> >> + (data).hdr.ioc_len = sizeof(data); \
>> >> +} while (0)
>> >> +
>> >
>> >Do we really need this?
>>
>> Would you be okay if this was a inline function? This is used by user land
>> and kernel space code.
>>
>
>I try (not very hard) to sound like a broken record but this business of
>sharing code with userland is a pain in the butt. It's not used in the
>kernel or in any patches you have sent.
>
>It would look better as an inline function though so I wouldn't have
>even noticed it.
I'm glad you noticed. I just looked at the production source code and yep it
is only used
in the userland tools code. I need to update our tools so they don't break.
Then we can
remove these macros.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] tools/hv: Use include/uapi with __EXPORTED_HEADERS__
On Tue, Dec 15, 2015 at 10:21:38AM -0800, Kamal Mostafa wrote: > >From f424410bf4a6a0dd6ff88bec808798016f4161a0 Mon Sep 17 00:00:00 2001 > From: Kamal Mostafa > Date: Wed, 11 Nov 2015 19:16:59 + > Subject: tools/hv: Use include/uapi with __EXPORTED_HEADERS__ Why is all of this mess here? Please don't do that... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: fix address space mismatches
On Tue, Dec 15, 2015 at 04:50:45PM +, Okash Khawaja wrote:
> This patch fixes address space warnings from sparse. Function
> lprocfs_write_helper() accepts user space buffer but was being
> passed kernel space buffer by these functions:
>
> contention_seconds_store()
> lockless_truncate_store()
>
> Since these functions are used to implement show and store functions of
> lustre_attr object and since lustre_attr object is used to implement object
> inheritance through use of `container_of`, the address space warnings
> show up at multiple places inside driver's code base.
>
> This patch creates a user space version of lustre_attr object lustre_attr_u.
> Keeping function names and signatures same - other than the __user attribute -
> ensures that object inheritance continues to work as it was, but address
> space discrepency is removed. That removes a whole bunch of address
> space warnings.
>
> Signed-off-by: Okash Khawaja
> ---
> drivers/staging/lustre/lustre/include/lprocfs_status.h | 16
> drivers/staging/lustre/lustre/osc/lproc_osc.c | 12 ++--
> 2 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/include/lprocfs_status.h
> b/drivers/staging/lustre/lustre/include/lprocfs_status.h
> index f18c0c7..df6d9d5 100644
> --- a/drivers/staging/lustre/lustre/include/lprocfs_status.h
> +++ b/drivers/staging/lustre/lustre/include/lprocfs_status.h
> @@ -698,6 +698,22 @@ static struct lustre_attr lustre_attr_##name =
> __ATTR(name, mode, show, store)
> #define LUSTRE_RO_ATTR(name) LUSTRE_ATTR(name, 0444, name##_show, NULL)
> #define LUSTRE_RW_ATTR(name) LUSTRE_ATTR(name, 0644, name##_show,
> name##_store)
>
> +struct lustre_attr_u {
> + struct attribute attr;
> + ssize_t (*show)(struct kobject *kobj, struct attribute *attr,
> + char *buf);
> + ssize_t (*store)(struct kobject *kobj, struct attribute *attr,
> + const char __user *buf, size_t len);
sysfs files do not have __user pointers, something is really wrong here
if that's the solution :(
See the other comments in the mailing list archives for how messed up
the __user and kernel pointers are in lustre, and how I'd not recommend
anyone trying to fix them, unless you are a lustre developer and can
test all of your changes...
sorry,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions
On Tue, Dec 15, 2015 at 07:02:31AM -0800, Joe Perches wrote: > This is the original code: > > result = foo(); > if (result) > goto label; > > result = bar(); > if (result) > goto label; > > result = baz(); > if (result) > goto label; > > label: > go on... > No. There is no test. The original code looks like: result = foo(); if (result) goto out; result = baz(); goto out; out: go on.. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 21/40] staging: lustre: improve LNet clean up code and API
On Tue, Dec 15, 2015 at 05:10:39PM +, Simmons, James A. wrote: >I have been pondering if pushing bug fixes before style cleanups > is the right thing to do. Generally push the least controversial patches first so that if you have to redo one patch, then the rest are already applied and don't need to be changed. > I pushed a bunch of bug fixes earlier and none got > merged which either means Greg is just backed up and hasn't the time to > merge them or style issues are higher priority I have no idea which patchset you are talking about so I can't comment. Greg always (except if there is a mistake) applies things in first come first serve order. He doesn't sort them. > Assuming these bug fixes are in scope of the staging tree. Should I > continue to push this work first? You've pushed a bunch of stuff. I don't know which stuff has been applied and which has not. If no one replied to it and there isn't a dire issue such as a compile failure or it doesn't apply then Greg is likely to apply it. He doesn't silently patches, so you will get an email either way. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command
On Tue, Dec 15, 2015 at 06:14:19PM +, Simmons, James A. wrote:
>
> >> struct libcfs_ioctl_hdr {
> >>__u32 ioc_len;
> >> @@ -87,6 +88,13 @@ do {\
> >>data.ioc_hdr.ioc_len = sizeof(data);\
> >> } while (0)
> >>
> >> +#define LIBCFS_IOC_INIT_V2(data, hdr) \
> >> +do { \
> >> + memset(&(data), 0, sizeof(data)); \
> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
> >> + (data).hdr.ioc_len = sizeof(data); \
> >> +} while (0)
> >> +
> >
> >Do we really need this?
>
> Would you be okay if this was a inline function? This is used by user
> land and kernel space code.
Then your code is broken, please never do that.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command
>> struct libcfs_ioctl_hdr {
>> __u32 ioc_len;
>> @@ -87,6 +88,13 @@ do { \
>> data.ioc_hdr.ioc_len = sizeof(data);\
>> } while (0)
>>
>> +#define LIBCFS_IOC_INIT_V2(data, hdr) \
>> +do {\
>> +memset(&(data), 0, sizeof(data)); \
>> +(data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> +(data).hdr.ioc_len = sizeof(data); \
>> +} while (0)
>> +
>
>Do we really need this?
Would you be okay if this was a inline function? This is used by user land and
kernel space code.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: lustre: Delete unnecessary goto statements in six functions
On Tue, 2015-12-15 at 19:49 +0100, SF Markus Elfring wrote: > > I think there should _not_ be a hardened rule. > I guess that it can become hard to achieve consensus on a precise rule. Consensus isn't unanimity. > I imagine that your feedback > can cause further software development troubles if the acceptance for > this update suggestion will really depend on the number of blank lines > before a jump label. The author of any particular bit of code can do whatever they want. Localized consistency is probably more valuable than global consistency. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: lustre: Delete unnecessary goto statements in six functions
> I think there should _not_ be a hardened rule. I guess that it can become hard to achieve consensus on a precise rule. > Style is just a guide. Generally nice … > Do what you think appropriate. I'm sorry for my evolving understanding. - But I imagine that your feedback can cause further software development troubles if the acceptance for this update suggestion will really depend on the number of blank lines before a jump label. Regards, Markus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: fix warning when releasing active sync point
2015-12-15 Daniel Vetter :
> On Mon, Dec 14, 2015 at 05:29:55PM -0800, Dmitry Torokhov wrote:
> > Userspace can close the sync device while there are still active fence
> > points, in which case kernel produces the following warning:
> >
> > [ 43.853176] [ cut here ]
> > [ 43.857834] WARNING: CPU: 0 PID: 892 at
> > /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
> > android_fence_release+0x88/0x104()
> > [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U
> > 3.18.0-07661-g0550ce9 #1
> > [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
> > [ 43.885834] Call trace:
> > [ 43.888294] [] dump_backtrace+0x0/0x10c
> > [ 43.893697] [] show_stack+0x10/0x1c
> > [ 43.898756] [] dump_stack+0x74/0xb8
> > [ 43.903814] [] warn_slowpath_common+0x84/0xb0
> > [ 43.909736] [] warn_slowpath_null+0x14/0x20
> > [ 43.915482] [] android_fence_release+0x84/0x104
> > [ 43.921582] [] fence_release+0x104/0x134
> > [ 43.927066] [] sync_fence_free+0x74/0x9c
> > [ 43.932552] [] sync_fence_release+0x34/0x48
> > [ 43.938304] [] __fput+0x100/0x1b8
> > [ 43.943185] [] fput+0x8/0x14
> > [ 43.947982] [] task_work_run+0xb0/0xe4
> > [ 43.953297] [] do_notify_resume+0x44/0x5c
> > [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]---
> >
> > Let's fix it by introducing a new optional callback (disable_signaling)
> > to fence operations so that drivers can do proper clean ups when we
> > remove last callback for given fence.
> >
> > Reviewed-by: Andrew Bresticker
> > Signed-off-by: Dmitry Torokhov
> > ---
> > drivers/dma-buf/fence.c| 6 +-
> > drivers/staging/android/sync.c | 8
> > include/linux/fence.h | 2 ++
> > 3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
> > index 7b05dbe..0ed73ad 100644
> > --- a/drivers/dma-buf/fence.c
> > +++ b/drivers/dma-buf/fence.c
> > @@ -304,8 +304,12 @@ fence_remove_callback(struct fence *fence, struct
> > fence_cb *cb)
> > spin_lock_irqsave(fence->lock, flags);
> >
> > ret = !list_empty(&cb->node);
> > - if (ret)
> > + if (ret) {
> > list_del_init(&cb->node);
> > + if (list_empty(&fence->cb_list))
> > + if (fence->ops->disable_signaling)
> > + fence->ops->disable_signaling(fence);
>
> What exactly is the bug here? A fence with no callbacks registered any
> more shouldn't have any problem. Why exactly does this blow up?
The WARN_ON is probably this one:
https://android.googlesource.com/kernel/common/+/android-3.18/drivers/staging/android/sync.c#433
I've been wondering in the last few days if this warning is really
necessary. If the user is closing a sync_timeline that has unsignalled
fences it should probably be aware of that already. Then I think it is
okay to remove the the sync_pt from the active_list at the release-time.
In fact I've already prepared a patch doing that. Thoughts?
Gustavo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: lustre: Delete unnecessary goto statements in six functions
On Tue, 2015-12-15 at 19:26 +0100, SF Markus Elfring wrote: > > rc = mdc_queue_wait(req); > > goto out; > > out: > > ptlrpc_req_finished(req); > > return rc; > > } > > - > > > > I think if the last goto out; is to be removed, > > then it should be replaced by a blank line. > > > > It separates the last operation block from the return. > > Would you like to point a very specific coding style issue out? Other than using vertical separation can help readability? I think there should _not_ be a hardened rule. Style is just a guide. Do what you think appropriate. > How often should jump labels preceded with blank lines? When other nearby blocks are also separated by blank lines. Localized consistency can be useful. Inconsistency can make code harder to follow/predict. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: fix warning when releasing active sync point
On Tue, Dec 15, 2015 at 11:00 AM, Gustavo Padovan wrote:
> 2015-12-15 Daniel Vetter :
>
>> On Mon, Dec 14, 2015 at 05:29:55PM -0800, Dmitry Torokhov wrote:
>> > Userspace can close the sync device while there are still active fence
>> > points, in which case kernel produces the following warning:
>> >
>> > [ 43.853176] [ cut here ]
>> > [ 43.857834] WARNING: CPU: 0 PID: 892 at
>> > /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
>> > android_fence_release+0x88/0x104()
>> > [ 43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U
>> > 3.18.0-07661-g0550ce9 #1
>> > [ 43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
>> > [ 43.885834] Call trace:
>> > [ 43.888294] [] dump_backtrace+0x0/0x10c
>> > [ 43.893697] [] show_stack+0x10/0x1c
>> > [ 43.898756] [] dump_stack+0x74/0xb8
>> > [ 43.903814] [] warn_slowpath_common+0x84/0xb0
>> > [ 43.909736] [] warn_slowpath_null+0x14/0x20
>> > [ 43.915482] [] android_fence_release+0x84/0x104
>> > [ 43.921582] [] fence_release+0x104/0x134
>> > [ 43.927066] [] sync_fence_free+0x74/0x9c
>> > [ 43.932552] [] sync_fence_release+0x34/0x48
>> > [ 43.938304] [] __fput+0x100/0x1b8
>> > [ 43.943185] [] fput+0x8/0x14
>> > [ 43.947982] [] task_work_run+0xb0/0xe4
>> > [ 43.953297] [] do_notify_resume+0x44/0x5c
>> > [ 43.958867] ---[ end trace 5a2aa4027cc5d171 ]---
>> >
>> > Let's fix it by introducing a new optional callback (disable_signaling)
>> > to fence operations so that drivers can do proper clean ups when we
>> > remove last callback for given fence.
>> >
>> > Reviewed-by: Andrew Bresticker
>> > Signed-off-by: Dmitry Torokhov
>> > ---
>> > drivers/dma-buf/fence.c| 6 +-
>> > drivers/staging/android/sync.c | 8
>> > include/linux/fence.h | 2 ++
>> > 3 files changed, 15 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
>> > index 7b05dbe..0ed73ad 100644
>> > --- a/drivers/dma-buf/fence.c
>> > +++ b/drivers/dma-buf/fence.c
>> > @@ -304,8 +304,12 @@ fence_remove_callback(struct fence *fence, struct
>> > fence_cb *cb)
>> > spin_lock_irqsave(fence->lock, flags);
>> >
>> > ret = !list_empty(&cb->node);
>> > - if (ret)
>> > + if (ret) {
>> > list_del_init(&cb->node);
>> > + if (list_empty(&fence->cb_list))
>> > + if (fence->ops->disable_signaling)
>> > + fence->ops->disable_signaling(fence);
>>
>> What exactly is the bug here? A fence with no callbacks registered any
>> more shouldn't have any problem. Why exactly does this blow up?
>
> The WARN_ON is probably this one:
> https://android.googlesource.com/kernel/common/+/android-3.18/drivers/staging/android/sync.c#433
>
> I've been wondering in the last few days if this warning is really
> necessary. If the user is closing a sync_timeline that has unsignalled
> fences it should probably be aware of that already. Then I think it is
> okay to remove the the sync_pt from the active_list at the release-time.
> In fact I've already prepared a patch doing that. Thoughts?
>
Maybe, but you need to make sure that you only affecting your fences.
My main objection is that still leaves fence_remove_callback() being
not mirror image of fence_add_callback().
--
Dmitry
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: lustre: Delete unnecessary goto statements in six functions
On Tue, 2015-12-15 at 19:02 +0100, SF Markus Elfring wrote:
> > This is the original code:
> Really …?
> > result = baz();
> > if (result)
> > goto label;
> >
> > label:
> > go on...
>
> I do not see such a source code structure
> at the six places I propose to clean-up.
>
>
> > I don't find the test->goto label; label: use offensive,
> > but if he does, I think keeping a blank line in place of
> > the test->goto might be better.
>
> I find this an interesting view on source code layout.
> Are there any more opinions around such implementation details?
Or to put it another way, use a blank line before the
first or only label in an error/out block.
I don't find it different then commonly written blocks like:
void foo(void)
{
...;
wind1();
val = func1(...);
if (val) {
printk(...);
goto err_type;
}
wind2();
val = func2(...);
if (val) {
printk(...);
goto err_type2;
}
...
return 0;
err_type2:
unwind2();
err_type:
unwind1();
return -ERR;
}
Yes, you can elide all the blank lines, but using them can
help readability.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions
On Tue, 2015-12-15 at 20:48 +0300, Dan Carpenter wrote:
> On Tue, Dec 15, 2015 at 07:02:31AM -0800, Joe Perches wrote:
> > This is the original code:
> >
> > result = foo();
> > if (result)
> > goto label;
> >
> > result = bar();
> > if (result)
> > goto label;
> >
> > result = baz();
> > if (result)
> > goto label;
> >
> > label:
> > go on...
> >
>
> No. There is no test. The original code looks like:
>
> result = foo();
> if (result)
> goto out;
> result = baz();
> goto out;
> out:
> go on..
>
> regards,
> dan carpenter
Here is the original code:
-
/* Copy hsm_progress struct */
req_hpk = req_capsule_client_get(&req->rq_pill, &RMF_MDS_HSM_PROGRESS);
if (req_hpk == NULL) {
rc = -EPROTO;
goto out;
}
*req_hpk = *hpk;
req_hpk->hpk_errval = lustre_errno_hton(hpk->hpk_errval);
ptlrpc_request_set_replen(req);
rc = mdc_queue_wait(req);
goto out;
out:
ptlrpc_req_finished(req);
return rc;
}
-
I think if the last goto out; is to be removed,
then it should be replaced by a blank line.
It separates the last operation block from the return.
cheers, Joe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command
>On Tue, Dec 15, 2015 at 06:14:19PM +, Simmons, James A. wrote:
>>
>> >> struct libcfs_ioctl_hdr {
>> >> __u32 ioc_len;
>> >> @@ -87,6 +88,13 @@ do { \
>> >> data.ioc_hdr.ioc_len = sizeof(data);\
>> >> } while (0)
>> >>
>> >> +#define LIBCFS_IOC_INIT_V2(data, hdr)\
>> >> +do { \
>> >> + memset(&(data), 0, sizeof(data)); \
>> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> >> + (data).hdr.ioc_len = sizeof(data); \
>> >> +} while (0)
>> >> +
>> >
>> >Do we really need this?
>>
>> Would you be okay if this was a inline function? This is used by user
>> land and kernel space code.
>
>Then your code is broken, please never do that.
This brings up a good point. This header doesn't contain structures for
userland so it is a uapi
type header. Should such headers only contain data structures?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command
On Tue, Dec 15, 2015 at 07:48:22PM +, Simmons, James A. wrote:
> >On Tue, Dec 15, 2015 at 06:14:19PM +, Simmons, James A. wrote:
> >>
> >> >> struct libcfs_ioctl_hdr {
> >> >> __u32 ioc_len;
> >> >> @@ -87,6 +88,13 @@ do { \
> >> >> data.ioc_hdr.ioc_len = sizeof(data);\
> >> >> } while (0)
> >> >>
> >> >> +#define LIBCFS_IOC_INIT_V2(data, hdr) \
> >> >> +do { \
> >> >> + memset(&(data), 0, sizeof(data)); \
> >> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
> >> >> + (data).hdr.ioc_len = sizeof(data); \
> >> >> +} while (0)
> >> >> +
> >> >
> >> >Do we really need this?
> >>
> >> Would you be okay if this was a inline function? This is used by user
> >> land and kernel space code.
> >
> >Then your code is broken, please never do that.
>
> This brings up a good point. This header doesn't contain structures for
> userland so it is a uapi
> type header. Should such headers only contain data structures?
Yes, that would make more sense.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: fix address space mismatches
> On 15 Dec 2015, at 18:48, Greg KH wrote:
>
>> On Tue, Dec 15, 2015 at 04:50:45PM +, Okash Khawaja wrote:
>> This patch fixes address space warnings from sparse. Function
>> lprocfs_write_helper() accepts user space buffer but was being
>> passed kernel space buffer by these functions:
>>
>> contention_seconds_store()
>> lockless_truncate_store()
>>
>> Since these functions are used to implement show and store functions of
>> lustre_attr object and since lustre_attr object is used to implement object
>> inheritance through use of `container_of`, the address space warnings
>> show up at multiple places inside driver's code base.
>>
>> This patch creates a user space version of lustre_attr object lustre_attr_u.
>> Keeping function names and signatures same - other than the __user attribute
>> -
>> ensures that object inheritance continues to work as it was, but address
>> space discrepency is removed. That removes a whole bunch of address
>> space warnings.
>>
>> Signed-off-by: Okash Khawaja
>> ---
>> drivers/staging/lustre/lustre/include/lprocfs_status.h | 16
>> drivers/staging/lustre/lustre/osc/lproc_osc.c | 12 ++--
>> 2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/include/lprocfs_status.h
>> b/drivers/staging/lustre/lustre/include/lprocfs_status.h
>> index f18c0c7..df6d9d5 100644
>> --- a/drivers/staging/lustre/lustre/include/lprocfs_status.h
>> +++ b/drivers/staging/lustre/lustre/include/lprocfs_status.h
>> @@ -698,6 +698,22 @@ static struct lustre_attr lustre_attr_##name =
>> __ATTR(name, mode, show, store)
>> #define LUSTRE_RO_ATTR(name) LUSTRE_ATTR(name, 0444, name##_show, NULL)
>> #define LUSTRE_RW_ATTR(name) LUSTRE_ATTR(name, 0644, name##_show,
>> name##_store)
>>
>> +struct lustre_attr_u {
>> +struct attribute attr;
>> +ssize_t (*show)(struct kobject *kobj, struct attribute *attr,
>> +char *buf);
>> +ssize_t (*store)(struct kobject *kobj, struct attribute *attr,
>> + const char __user *buf, size_t len);
>
> sysfs files do not have __user pointers, something is really wrong here
> if that's the solution :(
>
> See the other comments in the mailing list archives for how messed up
> the __user and kernel pointers are in lustre, and how I'd not recommend
> anyone trying to fix them, unless you are a lustre developer and can
> test all of your changes...
>
> sorry,
>
> greg k-h
I see. Thanks for getting back promptly.
Okash
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/3] Drivers: hv: Fix CPU assignment for FC devices
Currently all channels of a Fibre channel device are being bound to CPU 0. Correct this by including Fibre Channel devices in the list of performance critical devices. Also included is a patch to correct the returned error code for util drivers as well as cleanup of the vmbus signaling function. K. Y. Srinivasan (2): Drivers: hv: vmbus: Treat Fibre Channel devices as performance critical Drivers: hv: vmbus: Cleanup vmbus_set_event() Vitaly Kuznetsov (1): Drivers: hv: utils: fix hvt_op_poll() return value on transport destroy drivers/hv/channel_mgmt.c |3 +++ drivers/hv/connection.c |4 ++-- drivers/hv/hv.c | 16 drivers/hv/hv_utils_transport.c |2 +- drivers/hv/hyperv_vmbus.h |4 +--- 5 files changed, 7 insertions(+), 22 deletions(-) -- 1.7.4.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/3] Drivers: hv: utils: fix hvt_op_poll() return value on transport destroy
From: Vitaly Kuznetsov The return type of hvt_op_poll() is unsigned int and -EBADF is inappropriate, poll functions return POLL* statuses. Reported-by: Dexuan Cui Signed-off-by: Vitaly Kuznetsov Signed-off-by: K. Y. Srinivasan --- drivers/hv/hv_utils_transport.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c index ee20b50..4f42c0e 100644 --- a/drivers/hv/hv_utils_transport.c +++ b/drivers/hv/hv_utils_transport.c @@ -109,7 +109,7 @@ static unsigned int hvt_op_poll(struct file *file, poll_table *wait) poll_wait(file, &hvt->outmsg_q, wait); if (hvt->mode == HVUTIL_TRANSPORT_DESTROY) - return -EBADF; + return POLLERR | POLLHUP; if (hvt->outmsg_len > 0) return POLLIN | POLLRDNORM; -- 1.7.4.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/3] Drivers: hv: vmbus: Cleanup vmbus_set_event()
Signed-off-by: K. Y. Srinivasan
---
drivers/hv/connection.c |4 ++--
drivers/hv/hv.c | 16
drivers/hv/hyperv_vmbus.h |4 +---
3 files changed, 3 insertions(+), 21 deletions(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 3dc5a9c..4a320e6 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -474,7 +474,7 @@ int vmbus_post_msg(void *buffer, size_t buflen)
/*
* vmbus_set_event - Send an event notification to the parent
*/
-int vmbus_set_event(struct vmbus_channel *channel)
+void vmbus_set_event(struct vmbus_channel *channel)
{
u32 child_relid = channel->offermsg.child_relid;
@@ -485,5 +485,5 @@ int vmbus_set_event(struct vmbus_channel *channel)
(child_relid >> 5));
}
- return hv_signal_event(channel->sig_event);
+ hv_do_hypercall(HVCALL_SIGNAL_EVENT, channel->sig_event, NULL);
}
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 1db9556..2ed8e16 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -336,22 +336,6 @@ int hv_post_message(union hv_connection_id connection_id,
return status & 0x;
}
-
-/*
- * hv_signal_event -
- * Signal an event on the specified connection using the hypervisor event IPC.
- *
- * This involves a hypercall.
- */
-int hv_signal_event(void *con_id)
-{
- u64 status;
-
- status = hv_do_hypercall(HVCALL_SIGNAL_EVENT, con_id, NULL);
-
- return status & 0x;
-}
-
static int hv_ce_set_next_event(unsigned long delta,
struct clock_event_device *evt)
{
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 0411b7b..70540d5 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -587,8 +587,6 @@ extern int hv_post_message(union hv_connection_id
connection_id,
enum hv_message_type message_type,
void *payload, size_t payload_size);
-extern int hv_signal_event(void *con_id);
-
extern int hv_synic_alloc(void);
extern void hv_synic_free(void);
@@ -736,7 +734,7 @@ void vmbus_disconnect(void);
int vmbus_post_msg(void *buffer, size_t buflen);
-int vmbus_set_event(struct vmbus_channel *channel);
+void vmbus_set_event(struct vmbus_channel *channel);
void vmbus_on_event(unsigned long data);
--
1.7.4.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/3] Drivers: hv: vmbus: Treat Fibre Channel devices as performance critical
For performance critical devices, we distribute the incoming
channel interrupt load across available CPUs in the guest.
Include Fibre channel devices in the set of devices for which
we would distribute the interrupt load.
Signed-off-by: K. Y. Srinivasan
---
drivers/hv/channel_mgmt.c |3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index d013171..1c1ad47 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -361,6 +361,7 @@ err_free_chan:
enum {
IDE = 0,
SCSI,
+ FC,
NIC,
ND_NIC,
PCIE,
@@ -377,6 +378,8 @@ static const struct hv_vmbus_device_id hp_devs[] = {
{ HV_IDE_GUID, },
/* Storage - SCSI */
{ HV_SCSI_GUID, },
+ /* Storage - FC */
+ { HV_SYNTHFC_GUID, },
/* Network */
{ HV_NIC_GUID, },
/* NetworkDirect Guest RDMA */
--
1.7.4.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: Skein: Moved macros from skein_block.c to header file.
On Tue, 15 Dec 2015 07:55:15 -0700 Mathieu Poirier wrote: > I must admit you lost me here - what is this new version about? I > suggest you used the [PATCH v#] convention along with a log of > modifications from one version to another when sending new revisions. > That way people know what to look for. It moves the macros that were not nested inside functions to the header file. See how all the "defines" in the code are moved to skein_block.h? The patch just changes 7 lines. The change was on the TODO list, under the heading "Move macros to header files." ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 1/3] Drivers: hv: utils: fix hvt_op_poll() return value on transport destroy
> From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf > Of K. Y. Srinivasan > Sent: Wednesday, December 16, 2015 8:27 > To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; > vkuzn...@redhat.com; jasow...@redhat.com > Subject: [PATCH 1/3] Drivers: hv: utils: fix hvt_op_poll() return value on > transport > destroy > > From: Vitaly Kuznetsov > > The return type of hvt_op_poll() is unsigned int and -EBADF is > inappropriate, poll functions return POLL* statuses. > > Reported-by: Dexuan Cui > Signed-off-by: Vitaly Kuznetsov > Signed-off-by: K. Y. Srinivasan > --- > drivers/hv/hv_utils_transport.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c > index ee20b50..4f42c0e 100644 > --- a/drivers/hv/hv_utils_transport.c > +++ b/drivers/hv/hv_utils_transport.c > @@ -109,7 +109,7 @@ static unsigned int hvt_op_poll(struct file *file, > poll_table *wait) > poll_wait(file, &hvt->outmsg_q, wait); > > if (hvt->mode == HVUTIL_TRANSPORT_DESTROY) > - return -EBADF; > + return POLLERR | POLLHUP; > > if (hvt->outmsg_len > 0) > return POLLIN | POLLRDNORM; > -- Hi Vitaly, The daemon only polls on POLLIN. I'm not sure returning "POLLERR | POLLHUP" here can wake up the daemon or not. Thanks, -- Dexuan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
