All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jiri Slaby <jirislaby@gmail.com>
To: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
Cc: linuxppc-dev@ozlabs.org, git-dev <git-dev@xilinx.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Xilinx: hwicap: cleanup
Date: Mon, 25 Feb 2008 09:16:47 +0100	[thread overview]
Message-ID: <47C2796F.2070601@gmail.com> (raw)
In-Reply-To: <20080224232152.CEC5F12C8057@mail99-dub.bigfish.com>

On 02/25/2008 12:21 AM, Stephen Neuendorffer wrote:
>>>  @@ -549,8 +556,7 @@ static int hwicap_release(struct inode *inode,
> struct file *file)
>>>         int i;
>>>         int status = 0;
>>>
>>>  -       if (down_interruptible(&drvdata->sem))
>>>  -               return -ERESTARTSYS;
>>>  +       mutex_lock(&drvdata->sem);
>> Why not mutex_lock_interruptible()? (goes for all cases of
> mutex_lock())
> 
> It's not clear to me how to get 'correct' behavior in these functions if
> the interrupt happens.  For instance in probe/setup, if the mutex_lock
> is interrupted, it doesn't appear that there is anything to do other
> than return an error code that no device is present?  I think this was
> suggested by Jiri...

Yeah, since ERESTARTSYS would be ignored from f_op->release (see __fput()), 
drv->probe (see really_probe() and probe_failed label); not even talking about 
void return value functions. In those cases, the device won't be de/initialized 
and might result in unwanted behaviour (multiple modprobes for one device, 
rmmod/insmod need if you hit the path in release etc.).

WARNING: multiple messages have this Message-ID (diff)
From: Jiri Slaby <jirislaby@gmail.com>
To: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
Cc: Grant Likely <grant.likely@secretlab.ca>,
	linuxppc-dev@ozlabs.org, git-dev <git-dev@xilinx.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Xilinx: hwicap: cleanup
Date: Mon, 25 Feb 2008 09:16:47 +0100	[thread overview]
Message-ID: <47C2796F.2070601@gmail.com> (raw)
In-Reply-To: <20080224232152.CEC5F12C8057@mail99-dub.bigfish.com>

On 02/25/2008 12:21 AM, Stephen Neuendorffer wrote:
>>>  @@ -549,8 +556,7 @@ static int hwicap_release(struct inode *inode,
> struct file *file)
>>>         int i;
>>>         int status = 0;
>>>
>>>  -       if (down_interruptible(&drvdata->sem))
>>>  -               return -ERESTARTSYS;
>>>  +       mutex_lock(&drvdata->sem);
>> Why not mutex_lock_interruptible()? (goes for all cases of
> mutex_lock())
> 
> It's not clear to me how to get 'correct' behavior in these functions if
> the interrupt happens.  For instance in probe/setup, if the mutex_lock
> is interrupted, it doesn't appear that there is anything to do other
> than return an error code that no device is present?  I think this was
> suggested by Jiri...

Yeah, since ERESTARTSYS would be ignored from f_op->release (see __fput()), 
drv->probe (see really_probe() and probe_failed label); not even talking about 
void return value functions. In those cases, the device won't be de/initialized 
and might result in unwanted behaviour (multiple modprobes for one device, 
rmmod/insmod need if you hit the path in release etc.).

  parent reply	other threads:[~2008-02-25  8:16 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1202437061-12691-1-git-send-email-stephen.neuendorffer@xilinx.com>
2008-02-11 18:24 ` [PATCH] Xilinx: hwicap: cleanup Stephen Neuendorffer
2008-02-24  6:16   ` Grant Likely
2008-02-24  6:20     ` Grant Likely
2008-02-24 23:21     ` Stephen Neuendorffer
2008-02-24 23:21       ` Stephen Neuendorffer
2008-02-24 23:34       ` [PATCH] [POWERPC] [v2] " Stephen Neuendorffer
2008-02-24 23:34         ` Stephen Neuendorffer
2008-02-27 18:20         ` Grant Likely
2008-02-27 18:20           ` Grant Likely
2008-02-25  8:16       ` Jiri Slaby [this message]
2008-02-25  8:16         ` [PATCH] " Jiri Slaby

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=47C2796F.2070601@gmail.com \
    --to=jirislaby@gmail.com \
    --cc=git-dev@xilinx.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=stephen.neuendorffer@xilinx.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.