Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Mon, Dec 11, 2017 at 02:12:28PM -0800, Joe Perches wrote: > On Tue, 2017-12-12 at 08:43 +1100, Dave Chinner wrote: > > On Sat, Dec 09, 2017 at 09:00:18AM -0800, Joe Perches wrote: > > > On Sat, 2017-12-09 at 09:36 +1100, Dave Chinner wrote: > > > > 1. Using lockdep_set_novalidate_class() for anything other > > > > than device->mutex will throw checkpatch warnings. Nice. (*) > > > [] > > > > (*) checkpatch.pl is considered mostly harmful round here, too, > > > > but that's another rant > > > > > > How so? > > > > Short story is that it barfs all over the slightly non-standard > > coding style used in XFS. > [] > > This sort of stuff is just lowest-common-denominator noise - great > > for new code and/or inexperienced developers, but not for working > > with large bodies of existing code with slightly non-standard > > conventions. > > Completely reasonable. Thanks. > > Do you get many checkpatch submitters for fs/xfs? We used to. Not recently, though. Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Sun, Dec 10, 2017 at 08:23:15PM -0800, Matthew Wilcox wrote:
> On Mon, Dec 11, 2017 at 10:57:45AM +1100, Dave Chinner wrote:
> > i.e. the fact the cmpxchg failed may not have anything to do with a
> > race condtion - it failed because the slot wasn't empty like we
> > expected it to be. There can be any number of reasons the slot isn't
> > empty - the API should not "document" that the reason the insert
> > failed was a race condition. It should document the case that we
> > "couldn't insert because there was an existing entry in the slot".
> > Let the surrounding code document the reason why that might have
> > happened - it's not for the API to assume reasons for failure.
> >
> > i.e. this API and potential internal implementation makes much
> > more sense:
> >
> > int
> > xa_store_iff_empty(...)
> > {
> > curr = xa_cmpxchg(>pag_ici_xa, agino, NULL, ip, GFP_NOFS);
> > if (!curr)
> > return 0; /* success! */
> > if (!IS_ERR(curr))
> > return -EEXIST; /* failed - slot not empty */
> > return PTR_ERR(curr); /* failed - XA internal issue */
> > }
> >
> > as it replaces the existing preload and insert code in the XFS code
> > paths whilst letting us handle and document the "insert failed
> > because slot not empty" case however we want. It implies nothing
> > about *why* the slot wasn't empty, just that we couldn't do the
> > insert because it wasn't empty.
>
> Yeah, OK. So, over the top of the recent changes I'm looking at this:
>
> I'm not in love with xa_store_empty() as a name. I almost want
> xa_store_weak(), but after my MAP_FIXED_WEAK proposed name got shot
> down, I'm leery of it. "empty" is at least a concept we already have
> in the API with the comment for xa_init() talking about an empty array
> and xa_empty(). I also considered xa_store_null and xa_overwrite_null
> and xa_replace_null(). Naming remains hard.
>
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 941f38bb94a4..586b43836905 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -451,7 +451,7 @@ xfs_iget_cache_miss(
> int flags,
> int lock_flags)
> {
> - struct xfs_inode*ip, *curr;
> + struct xfs_inode*ip;
> int error;
> xfs_agino_t agino = XFS_INO_TO_AGINO(mp, ino);
> int iflags;
> @@ -498,8 +498,7 @@ xfs_iget_cache_miss(
> xfs_iflags_set(ip, iflags);
>
> /* insert the new inode */
> - curr = xa_cmpxchg(>pag_ici_xa, agino, NULL, ip, GFP_NOFS);
> - error = __xa_race(curr, -EAGAIN);
> + error = xa_store_empty(>pag_ici_xa, agino, ip, GFP_NOFS, -EAGAIN);
> if (error)
> goto out_unlock;
Can we avoid encoding the error to be returned into the function
call? No other generic/library API does this, so this seems like a
highly unusual special snowflake approach. I just don't see how
creating a whole new error specification convention is justified
for the case where it *might* save a line or two of error checking
code in a caller?
I'd much prefer that the API defines the error on failure, and let
the caller handle that specific error however they need to like
every other library interface we have...
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Sat, Dec 09, 2017 at 09:00:18AM -0800, Joe Perches wrote: > On Sat, 2017-12-09 at 09:36 +1100, Dave Chinner wrote: > > 1. Using lockdep_set_novalidate_class() for anything other > > than device->mutex will throw checkpatch warnings. Nice. (*) > [] > > (*) checkpatch.pl is considered mostly harmful round here, too, > > but that's another rant > > How so? Short story is that it barfs all over the slightly non-standard coding style used in XFS. It generates enough noise on incidental things we aren't important that it complicates simple things. e.g. I just moved a block of defines from one header to another, and checkpatch throws about 10 warnings on that because of whitespace. I'm just moving code - I don't want to change it and it doesn't need to be modified because it is neat and easy to read and is obviously correct. A bunch of prototypes I added another parameter to gets warnings because it uses "unsigned" for an existing parameter that I'm not changing. And so on. This sort of stuff is just lowest-common-denominator noise - great for new code and/or inexperienced developers, but not for working with large bodies of existing code with slightly non-standard conventions. Churning *lots* of code we otherwise wouldn't touch just to shut up checkpatch warnings takes time, risks regressions and makes it harder to trace the history of the code when we are trying to track down bugs. Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Fri, Dec 08, 2017 at 03:01:31PM -0800, Matthew Wilcox wrote:
> On Thu, Dec 07, 2017 at 11:38:43AM +1100, Dave Chinner wrote:
> > > > cmpxchg is for replacing a known object in a store - it's not really
> > > > intended for doing initial inserts after a lookup tells us there is
> > > > nothing in the store. The radix tree "insert only if empty" makes
> > > > sense here, because it naturally takes care of lookup/insert races
> > > > via the -EEXIST mechanism.
> > > >
> > > > I think that providing xa_store_excl() (which would return -EEXIST
> > > > if the entry is not empty) would be a better interface here, because
> > > > it matches the semantics of lookup cache population used all over
> > > > the kernel
> > >
> > > I'm not thrilled with xa_store_excl(), but I need to think about that
> > > a bit more.
> >
> > Not fussed about the name - I just think we need a function that
> > matches the insert semantics of the code
>
> I think I have something that works better for you than returning -EEXIST
> (because you don't actually want -EEXIST, you want -EAGAIN):
>
> /* insert the new inode */
> - spin_lock(>pag_ici_lock);
> - error = radix_tree_insert(>pag_ici_root, agino, ip);
> - if (unlikely(error)) {
> - WARN_ON(error != -EEXIST);
> - XFS_STATS_INC(mp, xs_ig_dup);
> - error = -EAGAIN;
> - goto out_preload_end;
> - }
> - spin_unlock(>pag_ici_lock);
> - radix_tree_preload_end();
> + curr = xa_cmpxchg(>pag_ici_xa, agino, NULL, ip, GFP_NOFS);
> + error = __xa_race(curr, -EAGAIN);
> + if (error)
> + goto out_unlock;
>
> ...
>
> -out_preload_end:
> - spin_unlock(>pag_ici_lock);
> - radix_tree_preload_end();
> +out_unlock:
> + if (error == -EAGAIN)
> + XFS_STATS_INC(mp, xs_ig_dup);
>
> I've changed the behaviour slightly in that returning an -ENOMEM used to
> hit a WARN_ON, and I don't think that's the right response -- GFP_NOFS
> returning -ENOMEM probably gets you a nice warning already from the
> mm code.
It's been a couple of days since I first looked at this, and my
initial reaction of "that's horrible!" hasn't changed.
What you are proposing here might be a perfectly reasonable
*internal implemention* of xa_store_excl(), but it makes for a
terrible external API because the sematics and behaviour are so
vague. e.g. what does "race" mean here with respect to an insert
failure?
i.e. the fact the cmpxchg failed may not have anything to do with a
race condtion - it failed because the slot wasn't empty like we
expected it to be. There can be any number of reasons the slot isn't
empty - the API should not "document" that the reason the insert
failed was a race condition. It should document the case that we
"couldn't insert because there was an existing entry in the slot".
Let the surrounding code document the reason why that might have
happened - it's not for the API to assume reasons for failure.
i.e. this API and potential internal implementation makes much
more sense:
int
xa_store_iff_empty(...)
{
curr = xa_cmpxchg(>pag_ici_xa, agino, NULL, ip, GFP_NOFS);
if (!curr)
return 0; /* success! */
if (!IS_ERR(curr))
return -EEXIST; /* failed - slot not empty */
return PTR_ERR(curr); /* failed - XA internal issue */
}
as it replaces the existing preload and insert code in the XFS code
paths whilst letting us handle and document the "insert failed
because slot not empty" case however we want. It implies nothing
about *why* the slot wasn't empty, just that we couldn't do the
insert because it wasn't empty.
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Lockdep is less useful than it was
On Fri, Dec 08, 2017 at 10:14:38AM -0800, Matthew Wilcox wrote: > At the moment, the radix tree actively disables the RCU checking that > enabling lockdep would give us. It has to, because it has no idea what > lock protects any individual access to the radix tree. The XArray can > use the RCU checking because it knows that every reference is protected > by either the spinlock or the RCU lock. > > Dave was saying that he has a tree which has to be protected by a mutex > because of where it is in the locking hierarchy, and I was vigorously > declining his proposal of allowing him to skip taking the spinlock. Oh, I wasn't suggesting that you remove the internal tree locking because we need external locking. I was trying to point out that the internal locking doesn't remove the need for external locking, and that there are cases where smearing the internal lock outside the XA tree doesn't work, either. i.e. internal locking doesn't replace all the cases where external locking is required, and so it's less efficient than the existing radix tree code. What I was questioning was the value of replacing the radix tree code with a less efficient structure just to add lockdep validation to a tree that doesn't actually need any extra locking validation... Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Fri, Dec 08, 2017 at 12:35:07PM -0500, Alan Stern wrote: > On Fri, 8 Dec 2017, Byungchul Park wrote: > > > I'm sorry to hear that.. If I were you, I would also get > > annoyed. And.. thanks for explanation. > > > > But, I think assigning lock classes properly and checking > > relationship of the classes to detect deadlocks is reasonable. > > > > In my opinion about the common lockdep stuff, there are 2 > > problems on it. > > > > 1) Firstly, it's hard to assign lock classes *properly*. By > > default, it relies on the caller site of lockdep_init_map(), > > but we need to assign another class manually, where ordering > > rules are complicated so cannot rely on the caller site. That > > *only* can be done by experts of the subsystem. Sure, but that's not the issue here. The issue here is the lack of communication with subsystem experts and that the annotation complexity warnings given immediately by the subsystem experts were completely ignored... > > I think if they want to get benifit from lockdep, they have no > > choice but to assign classes manually with the domain knowledge, > > or use *lockdep_set_novalidate_class()* to invalidate locks > > making the developers annoyed and not want to use the checking > > for them. > > Lockdep's no_validate class is used when the locking patterns are too > complicated for lockdep to understand. Basically, it tells lockdep to > ignore those locks. Let me just point out two things here: 1. Using lockdep_set_novalidate_class() for anything other than device->mutex will throw checkpatch warnings. Nice. (*) 2. lockdep_set_novalidate_class() is completely undocumented - it's the first I've ever heard of this functionality. i.e. nobody has ever told us there is a mechanism to turn off validation of an object; we've *always* been told to "change your code and/or fix your annotations" when discussing lockdep deficiencies. (**) > The device core uses that class. The tree of struct devices, each with > its own lock, gets used in many different and complicated ways. > Lockdep can't understand this -- it doesn't have the ability to > represent an arbitrarily deep hierarchical tree of locks -- so we tell ^^ That largely describes the in-memory structure of XFS, except we have a forest of lock trees, not just one > it to ignore the device locks. > > It sounds like XFS may need to do the same thing with its semaphores. Who-ever adds semaphore checking to lockdep can add those annotations. The externalisation of the development cost of new lockdep functionality is one of the problems here. -Dave. (*) checkpatch.pl is considered mostly harmful round here, too, but that's another rant (**) the frequent occurrence of "core code/devs aren't held to the same rules/standard as everyone else" is another rant I have stored up for a rainy day. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Fri, Dec 08, 2017 at 01:45:52PM +0900, Byungchul Park wrote: > On Fri, Dec 08, 2017 at 09:22:16AM +1100, Dave Chinner wrote: > > On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > > > On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote: > > > > > Unfortunately for you, I don't find arguments along the lines of > > > > > "lockdep will save us" at all convincing. lockdep already throws > > > > > too many false positives to be useful as a tool that reliably and > > > > > accurately points out rare, exciting, complex, intricate locking > > > > > problems. > > > > > > > > But it does reliably and accurately point out "dude, you forgot to take > > > > the lock". It's caught a number of real problems in my own testing that > > > > you never got to see. > > > > > > The problem is that if it has too many false positives --- and it's > > > gotten *way* worse with the completion callback "feature", people will > > > just stop using Lockdep as being too annyoing and a waste of developer > > > time when trying to figure what is a legitimate locking bug versus > > > lockdep getting confused. > > > > > > I can't even disable the new Lockdep feature which is throwing > > > lots of new false positives --- it's just all or nothing. > > > > > > Dave has just said he's already stopped using Lockdep, as a result. > > > > This is compeltely OT, but FYI I stopped using lockdep a long time > > ago. We've spend orders of magnitude more time and effort to shut > > up lockdep false positives in the XFS code than we ever have on > > locking problems that lockdep has uncovered. And still lockdep > > throws too many false positives on XFS workloads to be useful to me. > > > > But it's more than that: I understand just how much lockdep *doesn't > > check* and that means *I know I can't rely on lockdep* for potential > > deadlock detection. e.g. it doesn't cover semaphores, which means > > Hello, > > I'm careful in saying the following since you seem to feel not good at > crossrelease and even lockdep. Now that cross-release has been > introduced, semaphores can be covered as you might know. Actually, all > general waiters can. And all it will do is create a whole bunch more work for us XFS guys to shut up all the the false positive crap that falls out from it because the locking model we have is far more complex than any of the lockdep developers thought was necessary to support, just like happened with the XFS inode annotations all those years ago. e.g. nobody has ever bothered to ask us what is needed to describe XFS's semaphore locking model. If you did that, you'd know that we nest *thousands* of locked semaphores in compeltely random lock order during metadata buffer writeback. And that this lock order does not reflect the actual locking order rules we have for locking buffers during transactions. Oh, and you'd also know that a semaphore's lock order and context can change multiple times during the life time of the buffer. Say we free a block and the reallocate it as something else before it is reclaimed - that buffer now might have a different lock order. Or maybe we promote a buffer to be a root btree block as a result of a join - it's now the first buffer in a lock run, rather than a child. Or we split a tree, and the root is now a node and so no longer is the first buffer in a lock run. Or that we walk sideways along the leaf nodes siblings during searches. IOWs, there is no well defined static lock ordering at all for buffers - and therefore semaphores - in XFS at all. And knowing that, you wouldn't simply mention that lockdep can support semaphores now as though that is necessary to "make it work" for XFS. It's going to be much simpler for us to just turn off lockdep and ignore whatever crap it sends our way than it is to spend unplanned weeks of our time to try to make lockdep sorta work again. Sure, we might get there in the end, but it's likely to take months, if not years like it did with the XFS inode annotations. > > it has zero coverage of the entire XFS metadata buffer subsystem and > > the complex locking orders we have for metadata updates. > > > > Put simply: lockdep doesn't provide me with any benefit, so I don't > > use it... > > Sad.. I don't think you understand. I'll try to explain. The lockdep infrastructure by itself doesn't make lockdep a useful tool - it mostly generates false positives because it has no concept of locking models that don't match it's internal tracking assumptions and/or limitations. That means if we can't suppress the false positives, then l
Re: Lockdep is less useful than it was
On Thu, Dec 07, 2017 at 02:38:03PM -0800, Matthew Wilcox wrote: > On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > > The problem is that if it has too many false positives --- and it's > > gotten *way* worse with the completion callback "feature", people will > > just stop using Lockdep as being too annyoing and a waste of developer > > time when trying to figure what is a legitimate locking bug versus > > lockdep getting confused. > > > > I can't even disable the new Lockdep feature which is throwing > > lots of new false positives --- it's just all or nothing. > > You *can* ... but it's way more hacking Kconfig than you ought to have > to do (which is a separate rant ...) > > You need to get LOCKDEP_CROSSRELEASE off. I'd revert patches > e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and > b483cf3bc249d7af706390efa63d6671e80d1c09 > > I think it was a mistake to force these on for everybody; they have a > much higher false-positive rate than the rest of lockdep, so as you say > forcing them on leads to fewer people using *any* of lockdep. > > The bug you're hitting isn't Byungchul's fault; it's an annotation > problem. The same kind of annotation problem that we used to have with > dozens of other places in the kernel which are now fixed. That's one of the fundamental problem with lockdep - it throws the difficulty of solving all these new false positives onto the developers who know nothing about lockdep and don't follow it's development. And until they do solve them - especially in critical subsystems that everyone uses like the storage stack - lockdep is essentially worthless. > If you didn't > have to hack Kconfig to get rid of this problem, you'd be happier, right? I'd be much happier if it wasn't turned on by default in the first place. We gave plenty of warnings that there were still unsolved false positive problems with the new checks in the storage stack. Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote: > > > Unfortunately for you, I don't find arguments along the lines of > > > "lockdep will save us" at all convincing. lockdep already throws > > > too many false positives to be useful as a tool that reliably and > > > accurately points out rare, exciting, complex, intricate locking > > > problems. > > > > But it does reliably and accurately point out "dude, you forgot to take > > the lock". It's caught a number of real problems in my own testing that > > you never got to see. > > The problem is that if it has too many false positives --- and it's > gotten *way* worse with the completion callback "feature", people will > just stop using Lockdep as being too annyoing and a waste of developer > time when trying to figure what is a legitimate locking bug versus > lockdep getting confused. > > I can't even disable the new Lockdep feature which is throwing > lots of new false positives --- it's just all or nothing. > > Dave has just said he's already stopped using Lockdep, as a result. This is compeltely OT, but FYI I stopped using lockdep a long time ago. We've spend orders of magnitude more time and effort to shut up lockdep false positives in the XFS code than we ever have on locking problems that lockdep has uncovered. And still lockdep throws too many false positives on XFS workloads to be useful to me. But it's more than that: I understand just how much lockdep *doesn't check* and that means *I know I can't rely on lockdep* for potential deadlock detection. e.g. it doesn't cover semaphores, which means it has zero coverage of the entire XFS metadata buffer subsystem and the complex locking orders we have for metadata updates. Put simply: lockdep doesn't provide me with any benefit, so I don't use it... Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote:
> On Wed, Dec 06, 2017 at 07:44:04PM +1100, Dave Chinner wrote:
> > On Tue, Dec 05, 2017 at 08:45:49PM -0800, Matthew Wilcox wrote:
> > > That said, using xa_cmpxchg() in the dquot code looked like the right
> > > thing to do? Since we'd dropped the qi mutex and the ILOCK, it looks
> > > entirely reasonable for another thread to come in and set up the dquot.
> > > But I'm obviously quite ignorant of the XFS internals, so maybe there's
> > > something else going on that makes this essentially a "can't happen".
> >
> > It's no different to the inode cache code, which drops the RCU
> > lock on lookup miss, instantiates the new inode (maybe reading it
> > off disk), then locks the tree and attempts to insert it. Both cases
> > use "insert if empty, otherwise retry lookup from start" semantics.
>
> Ah. I had my focus set a little narrow on the inode cache code and didn't
> recognise the pattern.
>
> Why do you sleep for one jiffy after encountering a miss, then seeing
> someone else insert the inode for you?
The sleep is a backoff that allows whatever we raced with to
complete, be it a hit that raced with an inode being reclaimed and
removed, or a miss that raced with another insert. Ideally we'd
sleep on the XFS_INEW bit, similar to the vfs I_NEW flag, but it's
not quite that simple with the reclaim side of things...
> > cmpxchg is for replacing a known object in a store - it's not really
> > intended for doing initial inserts after a lookup tells us there is
> > nothing in the store. The radix tree "insert only if empty" makes
> > sense here, because it naturally takes care of lookup/insert races
> > via the -EEXIST mechanism.
> >
> > I think that providing xa_store_excl() (which would return -EEXIST
> > if the entry is not empty) would be a better interface here, because
> > it matches the semantics of lookup cache population used all over
> > the kernel
>
> I'm not thrilled with xa_store_excl(), but I need to think about that
> a bit more.
Not fussed about the name - I just think we need a function that
matches the insert semantics of the code
> > > I'm quite happy to have normal API variants that don't save/restore
> > > interrupts. Just need to come up with good names ... I don't think
> > > xa_store_noirq() is a good name, but maybe you do?
> >
> > I'd prefer not to have to deal with such things at all. :P
> >
> > How many subsystems actually require irq safety in the XA locking
> > code? Make them use irqsafe versions, not make everyone else use
> > "noirq" versions, as is the convention for the rest of the kernel
> > code
>
> Hard to say how many existing radix tree users require the irq safety.
The mapping tree requires it because it gets called from IO
completion contexts to clear page writeback state, but I don't know
about any of the others.
> Also hard to say how many potential users (people currently using
> linked lists, people using resizable arrays, etc) need irq safety.
> My thinking was "make it safe by default and let people who know better
> have a way to opt out", but there's definitely something to be said for
> "make it fast by default and let people who need the unusual behaviour
> type those extra few letters".
>
> So, you're arguing for providing xa_store(), xa_store_irq(), xa_store_bh()
> and xa_store_irqsafe()? (at least on demand, as users come to light?)
> At least the read side doesn't require any variants; everybody can use
> RCU for read side protection.
That would follow the pattern of the rest of the kernel APIs, though
I think it might be cleaner to simply state the locking requirement
to xa_init() and keep all those details completely internal rather
than encoding them into API calls. After all, the "irqsafe-ness" of
the locking needs to be consistent across the entire XA instance
> ("safe", not "save" because I wouldn't make the caller provide the
> "flags" argument).
>
> > > At least, not today. One of the future plans is to allow xa_nodes to
> > > be allocated from ZONE_MOVABLE. In order to do that, we have to be
> > > able to tell which lock protects any given node. With the XArray,
> > > we can find that out (xa_node->root->xa_lock); with the radix tree,
> > > we don't even know what kind of lock protects the tree.
> >
> > Yup, this is a prime example of why we shouldn't be creating
> > external dependencies by smearing the locking context outside the XA
> > structure itself. It's not a stretch to se
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Tue, Dec 05, 2017 at 08:45:49PM -0800, Matthew Wilcox wrote: > On Wed, Dec 06, 2017 at 02:14:56PM +1100, Dave Chinner wrote: > > > The other conversions use the normal API instead of the advanced API, so > > > all of this gets hidden away. For example, the inode cache does this: > > > > Ah, OK, that's not obvious from the code changes. :/ > > Yeah, it's a lot easier to understand (I think!) if you build the > docs in that tree and look at > file:///home/willy/kernel/xarray-3/Documentation/output/core-api/xarray.html > (mutatis mutandi). I've tried to tell a nice story about how to put > all the pieces together from the normal to the advanced API. > > > However, it's probably overkill for XFS. In all the cases, when we > > insert there should be no entry in the tree - the > > radix tree insert error handling code there was simply catching > > "should never happen" cases and handling it without crashing. > > I thought it was probably overkill to be using xa_cmpxchg() in the > pag_ici patch. I didn't want to take away your error handling as part > of the conversion, but I think a rational person implementing it today > would just call xa_store() and not even worry about the return value > except to check it for IS_ERR(). *nod* > That said, using xa_cmpxchg() in the dquot code looked like the right > thing to do? Since we'd dropped the qi mutex and the ILOCK, it looks > entirely reasonable for another thread to come in and set up the dquot. > But I'm obviously quite ignorant of the XFS internals, so maybe there's > something else going on that makes this essentially a "can't happen". It's no different to the inode cache code, which drops the RCU lock on lookup miss, instantiates the new inode (maybe reading it off disk), then locks the tree and attempts to insert it. Both cases use "insert if empty, otherwise retry lookup from start" semantics. cmpxchg is for replacing a known object in a store - it's not really intended for doing initial inserts after a lookup tells us there is nothing in the store. The radix tree "insert only if empty" makes sense here, because it naturally takes care of lookup/insert races via the -EEXIST mechanism. I think that providing xa_store_excl() (which would return -EEXIST if the entry is not empty) would be a better interface here, because it matches the semantics of lookup cache population used all over the kernel > > Now that I've looked at this, I have to say that having a return > > value of NULL meaning "success" is quite counter-intuitive. That's > > going to fire my "that looks so wrong" detector every time I look at > > the code and notice it's erroring out on a non-null return value > > that isn't a PTR_ERR case > > It's the same convention as cmpxchg(). I think it's triggering your > "looks so wrong" detector because it's fundamentally not the natural > thing to write. Most definitely the case, and this is why it's a really bad interface for the semantics we have. This how we end up with code that makes it easy for programmers to screw up pointer checks in error handling... :/ > I'm quite happy to have normal API variants that don't save/restore > interrupts. Just need to come up with good names ... I don't think > xa_store_noirq() is a good name, but maybe you do? I'd prefer not to have to deal with such things at all. :P How many subsystems actually require irq safety in the XA locking code? Make them use irqsafe versions, not make everyone else use "noirq" versions, as is the convention for the rest of the kernel code > > > It's the design pattern I've always intended to use. Naturally, the > > > xfs radix trees weren't my initial target; it was the page cache, and > > > the page cache does the same thing; uses the tree_lock to protect both > > > the radix tree and several other fields in that same data structure. > > > > > > I'm open to argument on this though ... particularly if you have a better > > > design pattern in mind! > > > > I don't mind structures having internal locking - I have a problem > > with leaking them into contexts outside the structure they protect. > > That way lies madness - you can't change the internal locking in > > future because of external dependencies, and the moment you need > > something different externally we've got to go back to an external > > lock anyway. > > > > This is demonstrated by the way you converted the XFS dquot tree - > > you didn't replace the dquot tree lock with the internal xa_lock > > because it's a mutex and we have to sleep holding it. IOWs, we've > > added another layer of locking here, not simplif
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Tue, Dec 05, 2017 at 06:02:08PM -0800, Matthew Wilcox wrote:
> On Wed, Dec 06, 2017 at 12:36:48PM +1100, Dave Chinner wrote:
> > > - if (radix_tree_preload(GFP_NOFS))
> > > - return -ENOMEM;
> > > -
> > > INIT_LIST_HEAD(>list_node);
> > > elem->key = key;
> > >
> > > - spin_lock(>lock);
> > > - error = radix_tree_insert(>store, key, elem);
> > > - radix_tree_preload_end();
> > > - if (!error)
> > > - _xfs_mru_cache_list_insert(mru, elem);
> > > - spin_unlock(>lock);
> > > + do {
> > > + xas_lock();
> > > + xas_store(, elem);
> > > + error = xas_error();
> > > + if (!error)
> > > + _xfs_mru_cache_list_insert(mru, elem);
> > > + xas_unlock();
> > > + } while (xas_nomem(, GFP_NOFS));
> >
> > Ok, so why does this have a retry loop on ENOMEM despite the
> > existing code handling that error? And why put such a loop in this
> > code and not any of the other XFS code that used
> > radix_tree_preload() and is arguably much more important to avoid
> > ENOMEM on insert (e.g. the inode cache)?
>
> If we need more nodes in the tree, xas_store() will try to allocate them
> with GFP_NOWAIT | __GFP_NOWARN. If that fails, it signals it in xas_error().
> xas_nomem() will notice that we're in an ENOMEM situation, and allocate
> a node using your preferred GFP flags (NOIO in your case). Then we retry,
> guaranteeing forward progress. [1]
>
> The other conversions use the normal API instead of the advanced API, so
> all of this gets hidden away. For example, the inode cache does this:
>
> + curr = xa_cmpxchg(>pag_ici_xa, agino, NULL, ip, GFP_NOFS);
>
> and xa_cmpxchg internally does:
>
> do {
> xa_lock_irqsave(xa, flags);
> curr = xas_create();
> if (curr == old)
> xas_store(, entry);
> xa_unlock_irqrestore(xa, flags);
> } while (xas_nomem(, gfp));
Ah, OK, that's not obvious from the code changes. :/
However, it's probably overkill for XFS. In all the cases, when we
insert there should be no entry in the tree - the
radix tree insert error handling code there was simply catching
"should never happen" cases and handling it without crashing.
Now that I've looked at this, I have to say that having a return
value of NULL meaning "success" is quite counter-intuitive. That's
going to fire my "that looks so wrong" detector every time I look at
the code and notice it's erroring out on a non-null return value
that isn't a PTR_ERR case
Also, there's no need for irqsave/restore() locking contexts here as
we never access these caches from interrupt contexts. That's just
going to be extra overhead, especially on workloads that run 10^6
inodes inodes through the cache every second. That's a problem
caused by driving the locks into the XA structure and then needing
to support callers that require irq safety
> > Also, I really don't like the pattern of using xa_lock()/xa_unlock()
> > to protect access to an external structure. i.e. the mru->lock
> > context is protecting multiple fields and operations in the MRU
> > structure, not just the radix tree operations. Turning that around
> > so that a larger XFS structure and algorithm is now protected by an
> > opaque internal lock from generic storage structure the forms part
> > of the larger structure seems like a bad design pattern to me...
>
> It's the design pattern I've always intended to use. Naturally, the
> xfs radix trees weren't my initial target; it was the page cache, and
> the page cache does the same thing; uses the tree_lock to protect both
> the radix tree and several other fields in that same data structure.
>
> I'm open to argument on this though ... particularly if you have a better
> design pattern in mind!
I don't mind structures having internal locking - I have a problem
with leaking them into contexts outside the structure they protect.
That way lies madness - you can't change the internal locking in
future because of external dependencies, and the moment you need
something different externally we've got to go back to an external
lock anyway.
This is demonstrated by the way you converted the XFS dquot tree -
you didn't replace the dquot tree lock with the internal xa_lock
because it's a mutex and we have to sleep holding it. IOWs, we've
added another layer of locking here, not simplified the code.
What I really see here is that we have inconsistent locking
patterns w.r.t. XA stores inside XFS - some have an external mutex
to cover a wider scope, some use xa_
Re: [PATCH v4 00/73] XArray version 4
On Tue, Dec 05, 2017 at 06:05:15PM -0800, Matthew Wilcox wrote: > On Wed, Dec 06, 2017 at 12:45:49PM +1100, Dave Chinner wrote: > > On Tue, Dec 05, 2017 at 04:40:46PM -0800, Matthew Wilcox wrote: > > > From: Matthew Wilcox <mawil...@microsoft.com> > > > > > > I looked through some notes and decided this was version 4 of the XArray. > > > Last posted two weeks ago, this version includes a *lot* of changes. > > > I'd like to thank Dave Chinner for his feedback, encouragement and > > > distracting ideas for improvement, which I'll get to once this is merged. > > > > BTW, you need to fix the "To:" line on your patchbombs: > > > > > To: unlisted-recipients: ;, no To-header on input > > > <@gmail-pop.l.google.com> > > > > This bad email address getting quoted to the cc line makes some MTAs > > very unhappy. > > I know :-( I was unhappy when I realised what I'd done. > > https://marc.info/?l=git=151252237912266=2 > > > I'll give this a quick burn this afternoon and see what catches fire... > > All of the things ... 0day gave me a 90% chance of hanging in one > configuration. Need to drill down on it more and find out what stupid > thing I've done wrong this time. Yup, Bad Stuff happened on boot: [ 24.548039] INFO: rcu_preempt detected stalls on CPUs/tasks: [ 24.548978] 1-...!: (0 ticks this GP) idle=688/0/0 softirq=143/143 fqs=0 [ 24.549926] 5-...!: (0 ticks this GP) idle=db8/0/0 softirq=120/120 fqs=0 [ 24.550864] 6-...!: (0 ticks this GP) idle=d58/0/0 softirq=111/111 fqs=0 [ 24.551802] 8-...!: (5 GPs behind) idle=514/0/0 softirq=189/189 fqs=0 [ 24.552722] 10-...!: (84 GPs behind) idle=ac0/0/0 softirq=80/80 fqs=0 [ 24.553617] 11-...!: (8 GPs behind) idle=cfc/0/0 softirq=95/95 fqs=0 [ 24.554496] 13-...!: (8 GPs behind) idle=b0c/0/0 softirq=82/82 fqs=0 [ 24.555382] 14-...!: (38 GPs behind) idle=a7c/0/0 softirq=93/93 fqs=0 [ 24.556305] 15-...!: (4 GPs behind) idle=b18/0/0 softirq=88/88 fqs=0 [ 24.557190] (detected by 9, t=5252 jiffies, g=-178, c=-179, q=994) [ 24.558051] Sending NMI from CPU 9 to CPUs 1: [ 24.558703] NMI backtrace for cpu 1 skipped: idling at native_safe_halt+0x2/0x10 [ 24.559654] Sending NMI from CPU 9 to CPUs 5: [ 24.559675] NMI backtrace for cpu 5 skipped: idling at native_safe_halt+0x2/0x10 [ 24.560654] Sending NMI from CPU 9 to CPUs 6: [ 24.560689] NMI backtrace for cpu 6 skipped: idling at native_safe_halt+0x2/0x10 [ 24.561655] Sending NMI from CPU 9 to CPUs 8: [ 24.561701] NMI backtrace for cpu 8 skipped: idling at native_safe_halt+0x2/0x10 [ 24.562654] Sending NMI from CPU 9 to CPUs 10: [ 24.562675] NMI backtrace for cpu 10 skipped: idling at native_safe_halt+0x2/0x10 [ 24.563653] Sending NMI from CPU 9 to CPUs 11: [ 24.563669] NMI backtrace for cpu 11 skipped: idling at native_safe_halt+0x2/0x10 [ 24.564653] Sending NMI from CPU 9 to CPUs 13: [ 24.564670] NMI backtrace for cpu 13 skipped: idling at native_safe_halt+0x2/0x10 [ 24.565652] Sending NMI from CPU 9 to CPUs 14: [ 24.565674] NMI backtrace for cpu 14 skipped: idling at native_safe_halt+0x2/0x10 [ 24.566652] Sending NMI from CPU 9 to CPUs 15: [ 24.59] NMI backtrace for cpu 15 skipped: idling at native_safe_halt+0x2/0x10 [ 24.567653] rcu_preempt kthread starved for 5256 jiffies! g18446744073709551438 c18446744073709551437 f0x0 RCU_GP_WAIT_FQS(3) ->state=0x402 ->7 [ 24.567654] rcu_preempt I15128 9 2 0x8000 [ 24.567660] Call Trace: [ 24.567679] ? __schedule+0x289/0x880 [ 24.567681] schedule+0x2f/0x90 [ 24.567682] schedule_timeout+0x152/0x370 [ 24.567686] ? __next_timer_interrupt+0xc0/0xc0 [ 24.567689] rcu_gp_kthread+0x561/0x880 [ 24.567691] ? force_qs_rnp+0x1a0/0x1a0 [ 24.567693] kthread+0x111/0x130 [ 24.567695] ? __kthread_create_worker+0x120/0x120 [ 24.567697] ret_from_fork+0x24/0x30 [ 44.064092] watchdog: BUG: soft lockup - CPU#0 stuck for 23s! [kswapd0:854] [ 44.065920] CPU: 0 PID: 854 Comm: kswapd0 Not tainted 4.15.0-rc2-dgc #228 [ 44.067769] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 [ 44.070030] RIP: 0010:smp_call_function_single+0xce/0x100 [ 44.071521] RSP: :c90001d2fb20 EFLAGS: 0202 ORIG_RAX: ff11 [ 44.073592] RAX: RBX: 88013ab515c8 RCX: c9000350bb20 [ 44.075560] RDX: 0001 RSI: c90001d2fb20 RDI: c90001d2fb20 [ 44.077531] RBP: c90001d2fb50 R08: 0007 R09: 0080 [ 44.079483] R10: c90001d2fb78 R11: c90001d2fb30 R12: c90001d2fc10 [ 44.081465] R13: ea000449fc78 R14: ea000449fc58 R15: 88013ba36c40 [ 44.083434] FS: () GS:88013fc0() knlGS: [ 44.085683] CS: 0010 DS: ES: CR0
Re: [PATCH v4 00/73] XArray version 4
On Wed, Dec 06, 2017 at 01:53:41AM +, Matthew Wilcox wrote: > Huh, you've caught a couple of problems that 0day hasn't sent me yet. Try > turning on DAX or TRANSPARENT_HUGEPAGE. Thanks! Dax is turned on, CONFIG_TRANSPARENT_HUGEPAGE is not. Looks like nothing is setting CONFIG_RADIX_TREE_MULTIORDER, which is what xas_set_order() is hidden under. Ah, CONFIG_ZONE_DEVICE turns it on, not CONFIG_DAX/CONFIG_FS_DAX. H. That seems wrong if it's used in fs/dax.c... $ grep DAX .config CONFIG_DAX=y CONFIG_FS_DAX=y $ grep ZONE_DEVICE .config CONFIG_ARCH_HAS_ZONE_DEVICE=y $ So I have DAX enabled, but not ZONE_DEVICE? Shouldn't DAX be selecting ZONE_DEVICE, not relying on a user to select both of them so that stuff works properly? Hmmm - there's no menu option to turn on zone device, so it's selected by something else? Oh, HMM turns on ZONE device. But that is "default y", so should be turned on. But it's not? And there's no obvious HMM menu config option, either What a godawful mess Kconfig has turned into. I'm just going to enable TRANSPARENT_HUGEPAGE - madness awaits me if I follow the other path down the rat hole Ok, it build this time. -Dave. > > > -Original Message- > > From: Dave Chinner [mailto:da...@fromorbit.com] > > Sent: Tuesday, December 5, 2017 8:51 PM > > To: Matthew Wilcox <wi...@infradead.org> > > Cc: Matthew Wilcox <mawil...@microsoft.com>; Ross Zwisler > > <ross.zwis...@linux.intel.com>; Jens Axboe <ax...@kernel.dk>; Rehas > > Sachdeva <aquan...@gmail.com>; linux...@kvack.org; linux- > > fsde...@vger.kernel.org; linux-f2fs-de...@lists.sourceforge.net; linux- > > ni...@vger.kernel.org; linux-bt...@vger.kernel.org; > > linux-...@vger.kernel.org; > > linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org > > Subject: Re: [PATCH v4 00/73] XArray version 4 > > > > On Wed, Dec 06, 2017 at 12:45:49PM +1100, Dave Chinner wrote: > > > On Tue, Dec 05, 2017 at 04:40:46PM -0800, Matthew Wilcox wrote: > > > > From: Matthew Wilcox <mawil...@microsoft.com> > > > > > > > > I looked through some notes and decided this was version 4 of the > > > > XArray. > > > > Last posted two weeks ago, this version includes a *lot* of changes. > > > > I'd like to thank Dave Chinner for his feedback, encouragement and > > > > distracting ideas for improvement, which I'll get to once this is > > > > merged. > > > > > > BTW, you need to fix the "To:" line on your patchbombs: > > > > > > > To: unlisted-recipients: ;, no To-header on input <@gmail- > > pop.l.google.com> > > > > > > This bad email address getting quoted to the cc line makes some MTAs > > > very unhappy. > > > > > > > > > > > Highlights: > > > > - Over 2000 words of documentation in patch 8! And lots more > > > > kernel-doc. > > > > - The page cache is now fully converted to the XArray. > > > > - Many more tests in the test-suite. > > > > > > > > This patch set is not for applying. 0day is still reporting problems, > > > > and I'd feel bad for eating someone's data. These patches apply on top > > > > of a set of prepatory patches which just aren't interesting. If you > > > > want to see the patches applied to a tree, I suggest pulling my git > > > > tree: > > > > > > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit.infrade > > ad.org%2Fusers%2Fwilly%2Flinux- > > dax.git%2Fshortlog%2Frefs%2Fheads%2Fxarray-2017-12- > > 04=02%7C01%7Cmawilcox%40microsoft.com%7Ca3e721545f8b4b9dff1 > > 608d53c4bd42f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6364 > > 81218740341312=IXNZXXLTf964OQ0eLDpJt2LCv%2BGGWFW%2FQd4Kc > > KYu6zo%3D=0 > > > > I also left out the idr_preload removals. They're still in the git > > > > tree, > > > > but I'm not looking for feedback on them. > > > > > > I'll give this a quick burn this afternoon and see what catches fire... > > > > Build warnings/errors: > > > > . > > lib/radix-tree.c:700:13: warning: ¿radix_tree_free_nodes¿ defined but not > > used > > [-Wunused-function] > > static void radix_tree_free_nodes(struct radix_tree_node *node) > > . > > lib/xarray.c: In function ¿xas_max¿: > > lib/xarray.c:291:16: warning: unused variable ¿mask¿ > > [-Wunused-variable] > > unsigned long mask, max = xas->xa_index; > > ^~~~ > > .. > > fs/dax.c: In function ¿grab_mapping_entry¿: > > fs/dax.c:305:2: error: implicit declaration of function ¿xas_set_order¿; > > did you > > mean ¿xas_set_err¿? [-Werror=implicit-function-declaration] > > xas_set_order(, index, size_flag ? PMD_ORDER : 0); > > ^ > > scripts/Makefile.build:310: recipe for target 'fs/dax.o' failed > > make[1]: *** [fs/dax.o] Error 1 > > > > -Dave. > > -- > > Dave Chinner > > da...@fromorbit.com -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/73] XArray version 4
On Wed, Dec 06, 2017 at 12:45:49PM +1100, Dave Chinner wrote: > On Tue, Dec 05, 2017 at 04:40:46PM -0800, Matthew Wilcox wrote: > > From: Matthew Wilcox <mawil...@microsoft.com> > > > > I looked through some notes and decided this was version 4 of the XArray. > > Last posted two weeks ago, this version includes a *lot* of changes. > > I'd like to thank Dave Chinner for his feedback, encouragement and > > distracting ideas for improvement, which I'll get to once this is merged. > > BTW, you need to fix the "To:" line on your patchbombs: > > > To: unlisted-recipients: ;, no To-header on input <@gmail-pop.l.google.com> > > This bad email address getting quoted to the cc line makes some MTAs > very unhappy. > > > > > Highlights: > > - Over 2000 words of documentation in patch 8! And lots more kernel-doc. > > - The page cache is now fully converted to the XArray. > > - Many more tests in the test-suite. > > > > This patch set is not for applying. 0day is still reporting problems, > > and I'd feel bad for eating someone's data. These patches apply on top > > of a set of prepatory patches which just aren't interesting. If you > > want to see the patches applied to a tree, I suggest pulling my git tree: > > http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/xarray-2017-12-04 > > I also left out the idr_preload removals. They're still in the git tree, > > but I'm not looking for feedback on them. > > I'll give this a quick burn this afternoon and see what catches fire... Build warnings/errors: . lib/radix-tree.c:700:13: warning: ¿radix_tree_free_nodes¿ defined but not used [-Wunused-function] static void radix_tree_free_nodes(struct radix_tree_node *node) . lib/xarray.c: In function ¿xas_max¿: lib/xarray.c:291:16: warning: unused variable ¿mask¿ [-Wunused-variable] unsigned long mask, max = xas->xa_index; ^~~~ .. fs/dax.c: In function ¿grab_mapping_entry¿: fs/dax.c:305:2: error: implicit declaration of function ¿xas_set_order¿; did you mean ¿xas_set_err¿? [-Werror=implicit-function-declaration] xas_set_order(, index, size_flag ? PMD_ORDER : 0); ^ scripts/Makefile.build:310: recipe for target 'fs/dax.o' failed make[1]: *** [fs/dax.o] Error 1 -Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/73] XArray version 4
On Tue, Dec 05, 2017 at 04:40:46PM -0800, Matthew Wilcox wrote: > From: Matthew Wilcox <mawil...@microsoft.com> > > I looked through some notes and decided this was version 4 of the XArray. > Last posted two weeks ago, this version includes a *lot* of changes. > I'd like to thank Dave Chinner for his feedback, encouragement and > distracting ideas for improvement, which I'll get to once this is merged. BTW, you need to fix the "To:" line on your patchbombs: > To: unlisted-recipients: ;, no To-header on input <@gmail-pop.l.google.com> This bad email address getting quoted to the cc line makes some MTAs very unhappy. > > Highlights: > - Over 2000 words of documentation in patch 8! And lots more kernel-doc. > - The page cache is now fully converted to the XArray. > - Many more tests in the test-suite. > > This patch set is not for applying. 0day is still reporting problems, > and I'd feel bad for eating someone's data. These patches apply on top > of a set of prepatory patches which just aren't interesting. If you > want to see the patches applied to a tree, I suggest pulling my git tree: > http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/xarray-2017-12-04 > I also left out the idr_preload removals. They're still in the git tree, > but I'm not looking for feedback on them. I'll give this a quick burn this afternoon and see what catches fire... Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Tue, Dec 05, 2017 at 04:41:58PM -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawil...@microsoft.com>
>
> This eliminates a call to radix_tree_preload().
.
> void
> @@ -431,24 +424,24 @@ xfs_mru_cache_insert(
> unsigned long key,
> struct xfs_mru_cache_elem *elem)
> {
> + XA_STATE(xas, >store, key);
> int error;
>
> ASSERT(mru && mru->lists);
> if (!mru || !mru->lists)
> return -EINVAL;
>
> - if (radix_tree_preload(GFP_NOFS))
> - return -ENOMEM;
> -
> INIT_LIST_HEAD(>list_node);
> elem->key = key;
>
> - spin_lock(>lock);
> - error = radix_tree_insert(>store, key, elem);
> - radix_tree_preload_end();
> - if (!error)
> - _xfs_mru_cache_list_insert(mru, elem);
> - spin_unlock(>lock);
> + do {
> + xas_lock();
> + xas_store(, elem);
> + error = xas_error();
> + if (!error)
> + _xfs_mru_cache_list_insert(mru, elem);
> + xas_unlock();
> + } while (xas_nomem(, GFP_NOFS));
Ok, so why does this have a retry loop on ENOMEM despite the
existing code handling that error? And why put such a loop in this
code and not any of the other XFS code that used
radix_tree_preload() and is arguably much more important to avoid
ENOMEM on insert (e.g. the inode cache)?
Also, I really don't like the pattern of using xa_lock()/xa_unlock()
to protect access to an external structure. i.e. the mru->lock
context is protecting multiple fields and operations in the MRU
structure, not just the radix tree operations. Turning that around
so that a larger XFS structure and algorithm is now protected by an
opaque internal lock from generic storage structure the forms part
of the larger structure seems like a bad design pattern to me...
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] HID: add quirk for another PIXART OEM mouse used by HP
This mouse keep disconnecting in runleve 3 like below, add it needs the
quirk to mute the anoying messages.
[ 111.230555] usb 2-2: USB disconnect, device number 6
[ 112.718156] usb 2-2: new low-speed USB device number 7 using xhci_hcd
[ 112.941594] usb 2-2: New USB device found, idVendor=03f0, idProduct=094a
[ 112.984866] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 113.027731] usb 2-2: Product: HP USB Optical Mouse
[ 113.069977] usb 2-2: Manufacturer: PixArt
[ 113.113500] input: PixArt HP USB Optical Mouse as
/devices/pci:00/:00:14.0/usb2/2-2/2-2:1.0/0003:03F0:094A.0002/input/input14
[ 113.156787] hid-generic 0003:03F0:094A.0002: input: USB HID v1.11 Mouse
[PixArt HP USB Optical Mouse] on usb-:00:14.0-2/input0
[ 173.262642] usb 2-2: USB disconnect, device number 7
[ 174.750244] usb 2-2: new low-speed USB device number 8 using xhci_hcd
[ 174.935740] usb 2-2: New USB device found, idVendor=03f0, idProduct=094a
[ 174.990435] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 175.014984] usb 2-2: Product: HP USB Optical Mouse
[ 175.037886] usb 2-2: Manufacturer: PixArt
[ 175.061794] input: PixArt HP USB Optical Mouse as
/devices/pci:00/:00:14.0/usb2/2-2/2-2:1.0/0003:03F0:094A.0003/input/input15
[ 175.084946] hid-generic 0003:03F0:094A.0003: input: USB HID v1.11 Mouse
[PixArt HP USB Optical Mouse] on usb-:00:14.0-2/input0
Signed-off-by: Dave Young <dyo...@redhat.com>
Cc: sta...@vger.kernel.org
---
v1->v2: rebase to usb tree hid-quirks-cleanup branch
drivers/hid/hid-ids.h|1 +
drivers/hid/hid-quirks.c |1 +
2 files changed, 2 insertions(+)
--- linux-x86.orig/drivers/hid/hid-ids.h
+++ linux-x86/drivers/hid/hid-ids.h
@@ -535,6 +535,7 @@
#define USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0A4A 0x0a4a
#define USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0B4A 0x0b4a
#define USB_PRODUCT_ID_HP_PIXART_OEM_USB_OPTICAL_MOUSE 0x134a
+#define USB_PRODUCT_ID_HP_PIXART_OEM_USB_OPTICAL_MOUSE_094A0x094a
#define USB_VENDOR_ID_HUION0x256c
#define USB_DEVICE_ID_HUION_TABLET 0x006e
--- linux-x86.orig/drivers/hid/hid-quirks.c
+++ linux-x86/drivers/hid/hid-quirks.c
@@ -90,6 +90,7 @@ const struct hid_device_id hid_quirks[]
{ HID_USB_DEVICE(USB_VENDOR_ID_HP,
USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0A4A), HID_QUIRK_ALWAYS_POLL },
{ HID_USB_DEVICE(USB_VENDOR_ID_HP,
USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0B4A), HID_QUIRK_ALWAYS_POLL },
{ HID_USB_DEVICE(USB_VENDOR_ID_HP,
USB_PRODUCT_ID_HP_PIXART_OEM_USB_OPTICAL_MOUSE), HID_QUIRK_ALWAYS_POLL },
+ { HID_USB_DEVICE(USB_VENDOR_ID_HP,
USB_PRODUCT_ID_HP_PIXART_OEM_USB_OPTICAL_MOUSE_094A), HID_QUIRK_ALWAYS_POLL },
{ HID_USB_DEVICE(USB_VENDOR_ID_IDEACOM, USB_DEVICE_ID_IDEACOM_IDC6680),
HID_QUIRK_MULTI_INPUT },
{ HID_USB_DEVICE(USB_VENDOR_ID_INNOMEDIA,
USB_DEVICE_ID_INNEX_GENESIS_ATARI), HID_QUIRK_MULTI_INPUT },
{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X),
HID_QUIRK_MULTI_INPUT },
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usbhid: add quirk for another PIXART OEM mouse used by HP
On 11/27/17 at 09:49am, Benjamin Tissoires wrote: > On Nov 27 2017 or thereabouts, Greg KH wrote: > > On Mon, Nov 27, 2017 at 09:16:31AM +0100, Benjamin Tissoires wrote: > > > On Nov 25 2017 or thereabouts, Dave Young wrote: > > > > This mouse keep disconnecting in runleve 3 like below, add it needs the > > > > quirk to mute the anoying messages. > > > > > > > > [ 111.230555] usb 2-2: USB disconnect, device number 6 > > > > [ 112.718156] usb 2-2: new low-speed USB device number 7 using xhci_hcd > > > > [ 112.941594] usb 2-2: New USB device found, idVendor=03f0, > > > > idProduct=094a > > > > [ 112.984866] usb 2-2: New USB device strings: Mfr=1, Product=2, > > > > SerialNumber=0 > > > > [ 113.027731] usb 2-2: Product: HP USB Optical Mouse > > > > [ 113.069977] usb 2-2: Manufacturer: PixArt > > > > [ 113.113500] input: PixArt HP USB Optical Mouse as > > > > /devices/pci:00/:00:14.0/usb2/2-2/2-2:1.0/0003:03F0:094A.0002/input/input14 > > > > [ 113.156787] hid-generic 0003:03F0:094A.0002: input: USB HID v1.11 > > > > Mouse [PixArt HP USB Optical Mouse] on usb-:00:14.0-2/input0 > > > > [ 173.262642] usb 2-2: USB disconnect, device number 7 > > > > [ 174.750244] usb 2-2: new low-speed USB device number 8 using xhci_hcd > > > > [ 174.935740] usb 2-2: New USB device found, idVendor=03f0, > > > > idProduct=094a > > > > [ 174.990435] usb 2-2: New USB device strings: Mfr=1, Product=2, > > > > SerialNumber=0 > > > > [ 175.014984] usb 2-2: Product: HP USB Optical Mouse > > > > [ 175.037886] usb 2-2: Manufacturer: PixArt > > > > [ 175.061794] input: PixArt HP USB Optical Mouse as > > > > /devices/pci:00/:00:14.0/usb2/2-2/2-2:1.0/0003:03F0:094A.0003/input/input15 > > > > [ 175.084946] hid-generic 0003:03F0:094A.0003: input: USB HID v1.11 > > > > Mouse [PixArt HP USB Optical Mouse] on usb-:00:14.0-2/input0 > > > > > > > > Signed-off-by: Dave Young <dyo...@redhat.com> > > > > --- > > > > drivers/hid/hid-ids.h |1 + > > > > drivers/hid/usbhid/hid-quirks.c |1 + > > > > > > Patch looks good and should be good for stable too, but in the file > > > hid-quirks.c file has been moved one folder up. Please rebase the patch > > > on top of > > > https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git/log/?h=for-4.16/hid-quirks-cleanup/_base > > > It should be merged in linux-next in the next few days normally. > > > > > > Jiri, how can we deal with such stable-ish patches? > > > > I can almost always handle things as simple as file movements, don't > > worry about that. If there's a problem, I'll be sure to let people know > > :) > > > > OK, thanks Greg. > > Dave, please rebase on top of the branch I mentioned before, and add > also the stable@ tag. Benjamin, will do. I posted this when I was in my daughter's class last weekend, maybe I can retest and repost it next Saturday at Dec 4th or earlier if I can find free time before that :) > > Cheers, > Benjamin Thanks Dave -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usbhid: add quirk for another PIXART OEM mouse used by HP
This mouse keep disconnecting in runleve 3 like below, add it needs the
quirk to mute the anoying messages.
[ 111.230555] usb 2-2: USB disconnect, device number 6
[ 112.718156] usb 2-2: new low-speed USB device number 7 using xhci_hcd
[ 112.941594] usb 2-2: New USB device found, idVendor=03f0, idProduct=094a
[ 112.984866] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 113.027731] usb 2-2: Product: HP USB Optical Mouse
[ 113.069977] usb 2-2: Manufacturer: PixArt
[ 113.113500] input: PixArt HP USB Optical Mouse as
/devices/pci:00/:00:14.0/usb2/2-2/2-2:1.0/0003:03F0:094A.0002/input/input14
[ 113.156787] hid-generic 0003:03F0:094A.0002: input: USB HID v1.11 Mouse
[PixArt HP USB Optical Mouse] on usb-:00:14.0-2/input0
[ 173.262642] usb 2-2: USB disconnect, device number 7
[ 174.750244] usb 2-2: new low-speed USB device number 8 using xhci_hcd
[ 174.935740] usb 2-2: New USB device found, idVendor=03f0, idProduct=094a
[ 174.990435] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 175.014984] usb 2-2: Product: HP USB Optical Mouse
[ 175.037886] usb 2-2: Manufacturer: PixArt
[ 175.061794] input: PixArt HP USB Optical Mouse as
/devices/pci:00/:00:14.0/usb2/2-2/2-2:1.0/0003:03F0:094A.0003/input/input15
[ 175.084946] hid-generic 0003:03F0:094A.0003: input: USB HID v1.11 Mouse
[PixArt HP USB Optical Mouse] on usb-:00:14.0-2/input0
Signed-off-by: Dave Young <dyo...@redhat.com>
---
drivers/hid/hid-ids.h |1 +
drivers/hid/usbhid/hid-quirks.c |1 +
2 files changed, 2 insertions(+)
--- linux-x86.orig/drivers/hid/hid-ids.h
+++ linux-x86/drivers/hid/hid-ids.h
@@ -535,6 +535,7 @@
#define USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0A4A 0x0a4a
#define USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0B4A 0x0b4a
#define USB_PRODUCT_ID_HP_PIXART_OEM_USB_OPTICAL_MOUSE 0x134a
+#define USB_PRODUCT_ID_HP_PIXART_OEM_USB_OPTICAL_MOUSE_094A0x094a
#define USB_VENDOR_ID_HUION0x256c
#define USB_DEVICE_ID_HUION_TABLET 0x006e
--- linux-x86.orig/drivers/hid/usbhid/hid-quirks.c
+++ linux-x86/drivers/hid/usbhid/hid-quirks.c
@@ -99,6 +99,7 @@ static const struct hid_blacklist {
{ USB_VENDOR_ID_HP,
USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0A4A, HID_QUIRK_ALWAYS_POLL },
{ USB_VENDOR_ID_HP,
USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0B4A, HID_QUIRK_ALWAYS_POLL },
{ USB_VENDOR_ID_HP, USB_PRODUCT_ID_HP_PIXART_OEM_USB_OPTICAL_MOUSE,
HID_QUIRK_ALWAYS_POLL },
+ { USB_VENDOR_ID_HP,
USB_PRODUCT_ID_HP_PIXART_OEM_USB_OPTICAL_MOUSE_094A, HID_QUIRK_ALWAYS_POLL },
{ USB_VENDOR_ID_IDEACOM, USB_DEVICE_ID_IDEACOM_IDC6680,
HID_QUIRK_MULTI_INPUT },
{ USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_C007,
HID_QUIRK_ALWAYS_POLL },
{ USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_C077,
HID_QUIRK_ALWAYS_POLL },
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb-core: Add MS_INTR_BINTERVAL USB quirk
Thank you for your very helpful answer. I really do appreciate it. It's possible that this device is using high speed because it offers a feature to transfer its internal clipboard to the host, and it allows that clipboard to contain lots of data. Interestingly, though, hidden within a usage note, they do observe that, so far, they've been unable to achieve a transfer speed faster than 5,000 characters in seven minutes. It looks to me like this is exactly due to their incorrect setting of bInterval. :-) I've sent them a note about it. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb-core: Add MS_INTR_BINTERVAL USB quirk
[quoted lines by Alan Stern on 2017/03/12 at 21:40 -0400] >No, I was wondering why an HID device would run at high speed. Both >you and Samuel implied that this was because it was a USB-2 device. >But that is not an adequate answer, because it is perfectly valid for a >USB-2 device to run at full speed. I think I've misunderstood something about how to interpret bInterval. I'm now suspecting that bInterval must be interpreted the new (mocro frame) way only if the device is operating at least at high speed, and that, even if the device advertizes itself as USB 2.0, bInterval must still be interpreted the old way if the device is operating at full or low speed. Is that correct? If the above is correct, how can we tell from usbfs which way to interpret bInterval? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb-core: Add MS_INTR_BINTERVAL USB quirk
[quoted lines by Alan Stern on 2017/03/12 at 21:31 -0400] >A device's speed is only partially related to its USB version. A >USB-1.1 device can run at low speed or full speed. A USB-2 device can >run at low, full, or high speed. And a USB-3 device can run at low, >full, high, or Super speed. Yes, I did know this, so maybe I misunderstood what you were wondering about. Were you wondering why 64ms was too long? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb-core: Add MS_INTR_BINTERVAL USB quirk
[quoted lines by Alan Stern on 2017/03/12 at 21:40 -0400] >No, I was wondering why an HID device would run at high speed. Both >you and Samuel implied that this was because it was a USB-2 device. >But that is not an adequate answer, because it is perfectly valid for a >USB-2 device to run at full speed. What should we look at to find out what speed it wants to operate at? We didn't look into it becuaase the real problem, from our perspective, was that the 64ms interval was being honoured by the host when it should be the 10ms as is literally in the endpoint descriptor. Our assumption was that since it says it's USB 2.0 then bInterval must be intterpreted in light of that regardless of the actual speed used for communication. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb-core: Add MS_INTR_BINTERVAL USB quirk
[quoted lines by Alan Stern on 2017/03/12 at 17:18 -0400] >Interesting. This is a high-speed device that mistakenly uses the >low/full-speed encoding for an interrupt bInterval value? Yes. >That's pretty unusual. Most HID devices (which includes the Braille >devices I have heard of) run at low speed, and a few of them run at >full speed. I can't remember any running at high speed. According to my collection of data, 5 say 1.00, 15 say 1.1, and 21 say 2.0. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usbtmc: Add, clarify and fix comments
Add information regarding lifespan of kref protection: Clarify comment on kref_get for interrupt in urb in usbtmc_probe() Add comment on kref_get in usbtmc_open() Fix endpoint reference in documentation for send_request_dev_dep_msg_in() Signed-off-by: Dave Penkler <dpenk...@gmail.com> --- drivers/usb/class/usbtmc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index a6c1fae..f03692e 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -157,6 +157,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) } data = usb_get_intfdata(intf); + /* Protect reference to data from file structure until release */ kref_get(>kref); /* Store pointer in file structure's private data field */ @@ -531,7 +532,7 @@ static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data, } /* - * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint. + * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-OUT endpoint. * @transfer_size: number of bytes to request from the device. * * See the USBTMC specification, Table 4. @@ -1471,7 +1472,7 @@ static int usbtmc_probe(struct usb_interface *intf, if (!data->iin_urb) goto error_register; - /* will reference data in int urb */ + /* Protect interrupt in endpoint data until iin_urb is freed */ kref_get(>kref); /* allocate buffer for interrupt in */ -- 2.9.2 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Wed, Sep 28, 2016 at 12:29:35PM +0200, Ladislav Michl wrote: > On Wed, Sep 28, 2016 at 11:45:02AM +0200, Greg Kroah-Hartman wrote: > > If it is freed, why is a read even able to happen? Ah, ick, no proper > > reference counting is happening here :( > > > > Oh, no, wait, it is happening properly, it's just that it's not the > > lifespan that the devm_kzalloc() is attached to, so yes, the correct fix > > here is to revert that patch as it is incorrect. > > Well, reference counting is also suspicious as kref_get(>kref) in > probe function with comment "will reference data in int urb" gives no > clue why there's explicit reference. Also what if we add classic > error unwinding and leave usb_submit_urb to open time? But wait, this > driver allows multiple opens? Is it intentional? It could be done this > way (note patch is only for reference as there's nothing to prevent > multiple open and therefore multiple usb_submit_urb): > The usbtmc_interrupt routine will reference data in the int urb from the interrupt context. Also submitting the urb on probe makes for simpler housekeeping. Only one interrupt urb needs to be submitted no matter how many concurrent opens there are for the minor. I will submit a patch to revert the "convert to devm_kzalloc" patch and make the comment more explicit. cheers, -dave -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: VL805 USB 3.0 does not see connected devices (only on x86_64) (x86 is ok)
On 09/12/2016 03:56 AM, Mathias Nyman wrote: > On 10.09.2016 17:19, Greg KH wrote: >> On Sat, Sep 10, 2016 at 04:33:05PM +0300, c400 wrote: >>> my DMESG file >> >>> [ 13.331618] usb 3-1: device descriptor read/64, error -110 >>> [ 28.535180] usb 3-1: device descriptor read/64, error -110 >>> [ 28.737931] usb 3-1: new high-speed USB device number 3 using >>> xhci_hcd >>> [ 33.840034] usb 3-1: device descriptor read/64, error -110 >>> [ 49.043616] usb 3-1: device descriptor read/64, error -110 >>> [ 49.246347] usb 3-1: new high-speed USB device number 4 using >>> xhci_hcd >>> [ 65.941816] xhci_hcd :02:00.0: Stopped the command ring >>> failed, maybe the host is dead >>> [ 65.965209] xhci_hcd :02:00.0: Host not halted after 16000 >>> microseconds. >>> [ 65.965211] xhci_hcd :02:00.0: Abort command ring failed >>> [ 65.965213] xhci_hcd :02:00.0: HC died; cleaning up >>> [ 65.965264] xhci_hcd :02:00.0: Timeout while waiting for setup >>> device command >> >> >> This is the bad part. >> >> Mathias, any ideas? I feel like we fixed something like this in the >> past for 64bit systems... >> > > There was at least xhci controllers on R-car SoCs that didn't support > 64bit DMA even if they claimed to. > > This could be 64bit DMA related as well. Before the usb device > descriptor read errors there is this in the lod: > > [ 10.124261] ioatdma: Intel(R) QuickData Technology Driver 4.00 > [ 10.124425] ioatdma :00:04.0: channel error register unreachable > [ 10.124427] ioatdma :00:04.0: channel enumeration error > [ 10.124430] ioatdma :00:04.0: Intel(R) I/OAT DMA Engine init failed > [ 10.124582] ioatdma :00:04.1: channel error register unreachable > [ 10.124583] ioatdma :00:04.1: channel enumeration error > [ 10.124586] ioatdma :00:04.1: Intel(R) I/OAT DMA Engine init failed > (for all channels up to :00:04.7) At least for this part is due to you don't have access to the extended PCI config space. The older kernels have a config option I believe and the newer kernels this should be by default enabled. I doubt they are related to your USB issues though. > > Could you (c400?) try forcing xhci to use 32bit DMA? it can be done by > setting XHCI_NO_64BIT_SUPPORT flag. > add 0x80 to quirks module paramterer when re-loading xhci. > In your case, load xhci with: > > sudo modprobe xhci_hcd quirks=0x00800090 > sudo modprobe xhci_pci > > Just to see if we can narrow it down to a 64 bit DMA related issue. > > I Added ioatdma people to cc in case those errors are relevant. > > Also adding extra xhci debugging could help pinpoint this: > echo -n 'module xhci_hcd =p' > /sys/kernel/debug/dynamic_debug/control > > -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: clear halt
[quoted lines by Greg KH on 2016/06/05 at 14:23 -0700] >> Using USBFS: If a device doesn't support clear halt (or, perhaps, not >> properly), is there an alternative? For example, is there a way to ask which >> state the device's data toggle is in and then tell the host what state to >> reset >> its end to? > >Nope, sorry, see the USB spec for details :( Thanks. I was just hoping that I'd missed something. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
clear halt
Using USBFS: If a device doesn't support clear halt (or, perhaps, not properly), is there an alternative? For example, is there a way to ask which state the device's data toggle is in and then tell the host what state to reset its end to? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ttyACM device issue.
Is this the right mailing list for ttyACM device issues, or should I be contactring the serial device people? Just in case this is the right place, here's the issue. I'm using Linux kernel 3.11.10-100.fc18.x86_64. I'm using termios, among other things, to turn ICANON off and to set both VTIME and VMIN to 0. This should do non-blocking reads that return 0 (something our code always successfully does with ttyS devices) but, with the ttyACM device, the read blocks. Is this expected behaviour for ttyACM devices? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: question on trust in chaoskey
> On May 19, 2016, at 10:59 PM, Dave Tian <dave.jing.t...@gmail.com> wrote: > > > >> On May 19, 2016, at 8:06 PM, Keith Packard <kei...@keithp.com> wrote: >> >> Oliver Neukum <oneu...@suse.com> writes: >> >>> I think we would need to use a form of public key cryptography >>> in the same manner used to verify authorship of emails. The host >>> would provide a nonce value that the device encrypts and returns. >>> The host would verify the signature. >> >> We could initially provision the devices with a unique key and provide >> the public half on a piece of paper. You'd have to get that into the >> kernel before the system needed any entropy though, and that seems hard. >> >> -- >> -keith > > Note that when we are talking about a nonce from host and a signature from > the device, we may potentially refer to TPM attestation. > The TPM has its own unique private key embedded in the hardware to derive > other keys, such as the attestation keys, > which can be used by the remote to verify the target’s firmware. > I am personally in favor of a TPM-like solution, since we probably > couldn’t/shouldn’t disable the firmware update anyway, > and we really need a hardware root of trust (with a key embedded) in the > device, like the TPM in the host. > > Whether this key should be provisioned by the user or the manufacture is > another interesting story. > Intel’s SGX[1] remote attestation is based on Intel’s EPID[2], which means if > the user want to make sure > the desire enclave code is running on an Intel SGX-enabled CPU, he/she has to > ask Intel for help, > since only Intel is able to verify if the attestation result was from a legit > CPU with SGX enabled. > Apparently, it does not sound good. So people did not like the patch[3]. > So Intel is going to add a user-defined root-of-trust key in the CPU, which > can be set during the boot time[4]. > Now the solution seems more decent, since now we do not have to deal with > Intel anyway. > BUT this does not mean Intel cannot do anything evil or users have a more > secure solution for CPU-based attestation. > > Credit card A let users to pick up a password - however, users have to be > responsible for identity thefts. > Credit card B does not have password at all - instead, users can dispute each > transaction. > Which one would you use? > > -daveti > > > [1]https://software.intel.com/en-us/sgx > [2]https://en.wikipedia.org/wiki/Enhanced_privacy_ID > [3]http://www.spinics.net/lists/linux-driver-devel/msg83272.html resend~ -daveti -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: usbtmc: Fix disconnect/poll interaction
When the device is disconnected poll waiters were not being woken.
Changes for v2:
- add commit summary
- add Fixes and Reported-by tags
Fixes: eb6b92ecc0f9 ("Add support for receiving USBTMC USB488 SRQ notifications
via poll/select")
Reported-by: Oliver Neukum <oneu...@suse.com>
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 419c72e..917a55c 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1525,13 +1525,14 @@ static void usbtmc_disconnect(struct usb_interface
*intf)
dev_dbg(>dev, "usbtmc_disconnect called\n");
data = usb_get_intfdata(intf);
- usbtmc_free_int(data);
usb_deregister_dev(intf, _class);
sysfs_remove_group(>dev.kobj, _attr_grp);
sysfs_remove_group(>dev.kobj, _attr_grp);
mutex_lock(>io_mutex);
data->zombie = 1;
+ wake_up_all(>waitq);
mutex_unlock(>io_mutex);
+ usbtmc_free_int(data);
kref_put(>kref, usbtmc_delete);
}
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: usbtmc: Fix disconnect/poll interaction
When the device is disconnected poll waiters were not being woken. Fixes issue in commit eb6b92ecc0f9412623ab1584ddd8389b371638d4 reported by Oliver Neukum Signed-off-by: Dave Penkler <dpenk...@gmail.com> --- drivers/usb/class/usbtmc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 419c72e..917a55c 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1525,13 +1525,14 @@ static void usbtmc_disconnect(struct usb_interface *intf) dev_dbg(>dev, "usbtmc_disconnect called\n"); data = usb_get_intfdata(intf); - usbtmc_free_int(data); usb_deregister_dev(intf, _class); sysfs_remove_group(>dev.kobj, _attr_grp); sysfs_remove_group(>dev.kobj, _attr_grp); mutex_lock(>io_mutex); data->zombie = 1; + wake_up_all(>waitq); mutex_unlock(>io_mutex); + usbtmc_free_int(data); kref_put(>kref, usbtmc_delete); } -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec
Hi Clemens, On Thu, Jan 28, 2016 at 09:46:59AM +0100, Clemens Ladisch wrote: > Dave Penkler wrote: > > Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) > > and SRQ notifications with fasync (2/5) and poll/select (3/5) in order > > to be able to synchronize with variable duration instrument > > operations. > > > > Add convenience ioctl to return all device capabilities (4/5) > > > > Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and > > LOCAL_LOCKOUT. (5/5) > > [...] > > Testing: > > All functions tested ok on an USBTMC-USB488 compliant oscilloscope > > How? Did you extend the usbtmc_ioctl tool? No, I have written my own test program and run it against my Agilent/Keysight Oscilloscope. It is available, as is, on request. cheers, -Dave -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec
Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) and SRQ notifications with fasync (2/5) and poll/select (3/5) in order to be able to synchronize with variable duration instrument operations. Add convenience ioctl to return all device capabilities (4/5) Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and LOCAL_LOCKOUT. (5/5) PATCH Changelog: v7 - Correct command direction and add data structure for USBTMC488_IOCTL_GET_CAPS - Cast arg to (void __user *) earlier in ioctl call chain - Correct type to __u8 for ioctl args to match unsigned char in userspace, 3 places: usb488_caps, stb and val v6 - Remove more excess newlines Rearrange declaration blocks aesthetically Remove __func__ parameter from dev_xxx calls Simplify tests for interrupt-in notifications Propagate return code from usb_submit_urb() v5 - Remove excess newlines and parens - Move dev variable initialisations to declaration - Add comment on interrupt btag wrap - simplify test in usbtmc_free_int() v4 - Remove excess newlines and parens - simplify some expressions v3 - Split into multiple patches as per gregkh request V2 - Fix V1 bug: not waking sleepers on disconnect. - Correct sparse warnings. V1 - Original patch Testing: All functions tested ok on an USBTMC-USB488 compliant oscilloscope Dave Penkler (5): Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Add support for USBTMC USB488 SRQ notification with fasync Add support for receiving USBTMC USB488 SRQ notifications via poll/select Add ioctl to retrieve USBTMC-USB488 capabilities Add ioctls to enable and disable local controls on an instrument drivers/usb/class/usbtmc.c | 330 +++ include/uapi/linux/usb/tmc.h | 29 +++- 2 files changed, 356 insertions(+), 3 deletions(-) -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 4/5] Add ioctl to retrieve USBTMC-USB488 capabilities
This is a convenience function to obtain an instrument's
capabilities from its file descriptor without having to access sysfs
from the user program.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 12
include/uapi/linux/usb/tmc.h | 21 ++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 8678429..379ba54 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -102,6 +102,9 @@ struct usbtmc_device_data {
u16iin_wMaxPacketSize;
atomic_t srq_asserted;
+ /* coalesced usb488_caps from usbtmc_dev_capabilities */
+ __u8 usb488_caps;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -993,6 +996,7 @@ static int get_capabilities(struct usbtmc_device_data *data)
data->capabilities.device_capabilities = buffer[5];
data->capabilities.usb488_interface_capabilities = buffer[14];
data->capabilities.usb488_device_capabilities = buffer[15];
+ data->usb488_caps = (buffer[14] & 0x07) | ((buffer[15] & 0x0f) << 4);
rv = 0;
err_out:
@@ -1168,6 +1172,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+ case USBTMC488_IOCTL_GET_CAPS:
+ retval = copy_to_user((void __user *)arg,
+ >usb488_caps,
+ sizeof(data->usb488_caps));
+ if (retval)
+ retval = -EFAULT;
+ break;
+
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, (void __user *)arg);
break;
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 7e5ced8..b512fb2 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -2,12 +2,14 @@
* Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
* Copyright (C) 2008 Novell, Inc.
* Copyright (C) 2008 Greg Kroah-Hartman <gre...@suse.de>
+ * Copyright (C) 2015 Dave Penkler <dpenk...@gmail.com>
*
* This file holds USB constants defined by the USB Device Class
- * Definition for Test and Measurement devices published by the USB-IF.
+ * and USB488 Subclass Definitions for Test and Measurement devices
+ * published by the USB-IF.
*
- * It also has the ioctl definitions for the usbtmc kernel driver that
- * userspace needs to know about.
+ * It also has the ioctl and capability definitions for the
+ * usbtmc kernel driver that userspace needs to know about.
*/
#ifndef __LINUX_USB_TMC_H
@@ -40,6 +42,19 @@
#define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4)
#define USBTMC_IOCTL_CLEAR_OUT_HALT_IO(USBTMC_IOC_NR, 6)
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
+#define USBTMC488_IOCTL_GET_CAPS _IOR(USBTMC_IOC_NR, 17, unsigned char)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+/* Driver encoded usb488 capabilities */
+#define USBTMC488_CAPABILITY_TRIGGER 1
+#define USBTMC488_CAPABILITY_SIMPLE 2
+#define USBTMC488_CAPABILITY_REN_CONTROL 2
+#define USBTMC488_CAPABILITY_GOTO_LOCAL 2
+#define USBTMC488_CAPABILITY_LOCAL_LOCKOUT 2
+#define USBTMC488_CAPABILITY_488_DOT_2 4
+#define USBTMC488_CAPABILITY_DT1 16
+#define USBTMC488_CAPABILITY_RL1 32
+#define USBTMC488_CAPABILITY_SR1 64
+#define USBTMC488_CAPABILITY_FULL_SCPI 128
+
#endif
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
Background:
When performing a read on an instrument that is executing a function
that runs longer than the USB timeout the instrument may hang and
require a device reset to recover. The READ_STATUS_BYTE operation
always returns even when the instrument is busy permitting to poll
for the appropriate condition. This capability is referred to in
instrument application notes on synchronizing acquisitions for other
platforms.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 201 +++
include/uapi/linux/usb/tmc.h | 2 +
2 files changed, 203 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..c2eb6fe 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
+ /* data for interrupt in endpoint handling */
+ u8 bNotify1;
+ u8 bNotify2;
+ u16ifnum;
+ u8 iin_bTag;
+ u8*iin_buffer;
+ atomic_t iin_data_valid;
+ unsigned int iin_ep;
+ intiin_ep_present;
+ intiin_interval;
+ struct urb*iin_urb;
+ u16iin_wMaxPacketSize;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -99,6 +112,7 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities capabilities;
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
+ wait_queue_head_t waitq;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -373,6 +387,84 @@ exit:
return rv;
}
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+ void __user *arg)
+{
+ struct device *dev = >intf->dev;
+ u8 *buffer;
+ u8 tag;
+ __u8 stb;
+ int rv;
+
+ dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+ data->iin_ep_present);
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ atomic_set(>iin_data_valid, 0);
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC488_REQUEST_READ_STATUS_BYTE,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ data->iin_bTag,
+ data->ifnum,
+ buffer, 0x03, USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (data->iin_ep_present) {
+ rv = wait_event_interruptible_timeout(
+ data->waitq,
+ atomic_read(>iin_data_valid) != 0,
+ USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_dbg(dev, "wait interrupted %d\n", rv);
+ goto exit;
+ }
+
+ if (rv == 0) {
+ dev_dbg(dev, "wait timed out\n");
+ rv = -ETIME;
+ goto exit;
+ }
+
+ tag = data->bNotify1 & 0x7f;
+ if (tag != data->iin_bTag) {
+ dev_err(dev, "expected bTag %x got %x\n",
+ data->iin_bTag, tag);
+ }
+
+ stb = data->bNotify2;
+ } else {
+ stb = buffer[2];
+ }
+
+ rv = copy_to_user(arg, , sizeof(stb));
+ if (rv)
+ rv = -EFAULT;
+
+ exit:
+ /* bump interrupt bTag */
+ data->iin_bTag += 1;
+ if (data->iin_bTag > 127)
+ /* 1 is for SRQ see USBTMC-USB488 subclass spec section 4.3.1 */
+ data->iin_bTag = 2;
+
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1069,6 +1161,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC_IOCTL_ABORT_BULK_IN:
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+
+ case USBTMC488_IOCTL_READ_STB:
+ retval = usbtmc488_ioctl_read_stb(data, (void __user *)arg);
+ break;
}
skip_io_on_zombie:
@@ -1092,6 +1188,57 @@ static st
[PATCH v7 3/5] Add support for receiving USBTMC USB488 SRQ notifications via poll/select
Background:
In many situations operations on multiple instruments need to be
synchronized. poll/select provide a convenient way of waiting on a
number of different instruments and other peripherals
simultaneously.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index d77da2f..8678429 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1184,6 +1185,27 @@ static int usbtmc_fasync(int fd, struct file *file, int
on)
return fasync_helper(fd, file, on, >fasync);
}
+static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
+{
+ struct usbtmc_device_data *data = file->private_data;
+ unsigned int mask;
+
+ mutex_lock(>io_mutex);
+
+ if (data->zombie) {
+ mask = POLLHUP | POLLERR;
+ goto no_poll;
+ }
+
+ poll_wait(file, >waitq, wait);
+
+ mask = (atomic_read(>srq_asserted)) ? POLLIN | POLLRDNORM : 0;
+
+no_poll:
+ mutex_unlock(>io_mutex);
+ return mask;
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1192,6 +1214,7 @@ static const struct file_operations fops = {
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
.fasync = usbtmc_fasync,
+ .poll = usbtmc_poll,
.llseek = default_llseek,
};
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 5/5] Add ioctls to enable and disable local controls on an instrument
These ioctls provide support for the USBTMC-USB488 control requests
for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 70
include/uapi/linux/usb/tmc.h | 6
2 files changed, 76 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 379ba54..bfc7515 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -474,6 +474,61 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
return rv;
}
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+ void __user *arg, unsigned int cmd)
+{
+ struct device *dev = >intf->dev;
+ __u8 val;
+ u8 *buffer;
+ u16 wValue;
+ int rv;
+
+ if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+ return -EINVAL;
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+ rv = copy_from_user(, arg, sizeof(val));
+ if (rv) {
+ rv = -EFAULT;
+ goto exit;
+ }
+ wValue = val ? 1 : 0;
+ } else {
+ wValue = 0;
+ }
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ cmd,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ wValue,
+ data->ifnum,
+ buffer, 0x01, USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+ goto exit;
+ } else if (rv != 1) {
+ dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "simple control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+ rv = 0;
+
+ exit:
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1183,6 +1238,21 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, (void __user *)arg);
break;
+
+ case USBTMC488_IOCTL_REN_CONTROL:
+ retval = usbtmc488_ioctl_simple(data, (void __user *)arg,
+ USBTMC488_REQUEST_REN_CONTROL);
+ break;
+
+ case USBTMC488_IOCTL_GOTO_LOCAL:
+ retval = usbtmc488_ioctl_simple(data, (void __user *)arg,
+ USBTMC488_REQUEST_GOTO_LOCAL);
+ break;
+
+ case USBTMC488_IOCTL_LOCAL_LOCKOUT:
+ retval = usbtmc488_ioctl_simple(data, (void __user *)arg,
+
USBTMC488_REQUEST_LOCAL_LOCKOUT);
+ break;
}
skip_io_on_zombie:
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index b512fb2..2e59d9c 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -33,6 +33,9 @@
#define USBTMC_REQUEST_GET_CAPABILITIES7
#define USBTMC_REQUEST_INDICATOR_PULSE 64
#define USBTMC488_REQUEST_READ_STATUS_BYTE 128
+#define USBTMC488_REQUEST_REN_CONTROL 160
+#define USBTMC488_REQUEST_GOTO_LOCAL 161
+#define USBTMC488_REQUEST_LOCAL_LOCKOUT162
/* Request values for USBTMC driver's ioctl entry point */
#define USBTMC_IOC_NR 91
@@ -44,6 +47,9 @@
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
#define USBTMC488_IOCTL_GET_CAPS _IOR(USBTMC_IOC_NR, 17, unsigned char)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+#define USBTMC488_IOCTL_REN_CONTROL_IOW(USBTMC_IOC_NR, 19, unsigned char)
+#define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20)
+#define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21)
/* Driver encoded usb488 capabilities */
#define USBTMC488_CAPABILITY_TRIGGER 1
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 4/5] Add ioctl to retrieve USBTMC-USB488 capabilities
On Sun, Jan 24, 2016 at 08:42:54PM -0800, Greg KH wrote:
> On Sun, Nov 29, 2015 at 01:35:51PM +0100, Dave Penkler wrote:
> > This is a convenience function to obtain an instrument's
> > capabilities from its file descriptor without having to access sysfs
> > from the user program.
> >
> > Signed-off-by: Dave Penkler <dpenk...@gmail.com>
> > ---
> > drivers/usb/class/usbtmc.c | 12
> > include/uapi/linux/usb/tmc.h | 21 ++---
> > 2 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > index 3b85ef5..3a3264c 100644
> > --- a/drivers/usb/class/usbtmc.c
> > +++ b/drivers/usb/class/usbtmc.c
> > @@ -102,6 +102,9 @@ struct usbtmc_device_data {
> > u16iin_wMaxPacketSize;
> > atomic_t srq_asserted;
> >
> > + /* coalesced usb488_caps from usbtmc_dev_capabilities */
> > + u8 usb488_caps;
> > +
> > u8 rigol_quirk;
> >
> > /* attributes from the USB TMC spec for this device */
> > @@ -992,6 +995,7 @@ static int get_capabilities(struct usbtmc_device_data
> > *data)
> > data->capabilities.device_capabilities = buffer[5];
> > data->capabilities.usb488_interface_capabilities = buffer[14];
> > data->capabilities.usb488_device_capabilities = buffer[15];
> > + data->usb488_caps = (buffer[14] & 0x07) | ((buffer[15] & 0x0f) << 4);
> > rv = 0;
> >
> > err_out:
> > @@ -1167,6 +1171,14 @@ static long usbtmc_ioctl(struct file *file, unsigned
> > int cmd, unsigned long arg)
> > retval = usbtmc_ioctl_abort_bulk_in(data);
> > break;
> >
> > + case USBTMC488_IOCTL_GET_CAPS:
> > + retval = copy_to_user((void __user *)arg,
> > + >usb488_caps,
> > + sizeof(data->usb488_caps));
> > + if (retval)
> > + retval = -EFAULT;
> > + break;
> > +
> > case USBTMC488_IOCTL_READ_STB:
> > retval = usbtmc488_ioctl_read_stb(data, arg);
> > break;
> > diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
> > index 7e5ced8..1dc3af1 100644
> > --- a/include/uapi/linux/usb/tmc.h
> > +++ b/include/uapi/linux/usb/tmc.h
> > @@ -2,12 +2,14 @@
> > * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
> > * Copyright (C) 2008 Novell, Inc.
> > * Copyright (C) 2008 Greg Kroah-Hartman <gre...@suse.de>
> > + * Copyright (C) 2015 Dave Penkler <dpenk...@gmail.com>
> > *
> > * This file holds USB constants defined by the USB Device Class
> > - * Definition for Test and Measurement devices published by the USB-IF.
> > + * and USB488 Subclass Definitions for Test and Measurement devices
> > + * published by the USB-IF.
> > *
> > - * It also has the ioctl definitions for the usbtmc kernel driver that
> > - * userspace needs to know about.
> > + * It also has the ioctl and capability definitions for the
> > + * usbtmc kernel driver that userspace needs to know about.
> > */
> >
> > #ifndef __LINUX_USB_TMC_H
> > @@ -40,6 +42,19 @@
> > #define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4)
> > #define USBTMC_IOCTL_CLEAR_OUT_HALT_IO(USBTMC_IOC_NR, 6)
> > #define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
> > +#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
>
> Shouldn't there be a data structure mentioned here that gets passed back
> to userspace? And are you sure the direction is correct as well?
>
Oops, it should read:
#define USBTMC488_IOCTL_GET_CAPS _IOR(USBTMC_IOC_NR, 17, unsigned char)
Thanks,
-dave
> thanks,
>
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
Background:
When performing a read on an instrument that is executing a function
that runs longer than the USB timeout the instrument may hang and
require a device reset to recover. The READ_STATUS_BYTE operation
always returns even when the instrument is busy permitting to poll
for the appropriate condition. This capability is referred to in
instrument application notes on synchronizing acquisitions for other
platforms.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 200 +++
include/uapi/linux/usb/tmc.h | 2 +
2 files changed, 202 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..89456df9 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
+ /* data for interrupt in endpoint handling */
+ u8 bNotify1;
+ u8 bNotify2;
+ u16ifnum;
+ u8 iin_bTag;
+ u8*iin_buffer;
+ atomic_t iin_data_valid;
+ unsigned int iin_ep;
+ intiin_ep_present;
+ intiin_interval;
+ struct urb*iin_urb;
+ u16iin_wMaxPacketSize;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -99,6 +112,7 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities capabilities;
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
+ wait_queue_head_t waitq;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -373,6 +387,83 @@ exit:
return rv;
}
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+ unsigned long arg)
+{
+ struct device *dev = >intf->dev;
+ u8 *buffer;
+ u8 tag, stb;
+ int rv;
+
+ dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+ data->iin_ep_present);
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ atomic_set(>iin_data_valid, 0);
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC488_REQUEST_READ_STATUS_BYTE,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ data->iin_bTag,
+ data->ifnum,
+ buffer, 0x03, USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (data->iin_ep_present) {
+ rv = wait_event_interruptible_timeout(
+ data->waitq,
+ atomic_read(>iin_data_valid) != 0,
+ USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_dbg(dev, "wait interrupted %d\n", rv);
+ goto exit;
+ }
+
+ if (rv == 0) {
+ dev_dbg(dev, "wait timed out\n");
+ rv = -ETIME;
+ goto exit;
+ }
+
+ tag = data->bNotify1 & 0x7f;
+ if (tag != data->iin_bTag) {
+ dev_err(dev, "expected bTag %x got %x\n",
+ data->iin_bTag, tag);
+ }
+
+ stb = data->bNotify2;
+ } else {
+ stb = buffer[2];
+ }
+
+ rv = copy_to_user((void __user *)arg, , sizeof(stb));
+ if (rv)
+ rv = -EFAULT;
+
+ exit:
+ /* bump interrupt bTag */
+ data->iin_bTag += 1;
+ if (data->iin_bTag > 127)
+ /* 1 is for SRQ see USBTMC-USB488 subclass spec section 4.3.1 */
+ data->iin_bTag = 2;
+
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1069,6 +1160,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC_IOCTL_ABORT_BULK_IN:
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+
+ case USBTMC488_IOCTL_READ_STB:
+ retval = usbtmc488_ioctl_read_stb(data, arg);
+ break;
}
skip_io_on_zombie:
@@ -1092,6 +1187,57 @@ static struct usb_class_driv
[PATCH v6 4/5] Add ioctl to retrieve USBTMC-USB488 capabilities
This is a convenience function to obtain an instrument's
capabilities from its file descriptor without having to access sysfs
from the user program.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 12
include/uapi/linux/usb/tmc.h | 21 ++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 3b85ef5..3a3264c 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -102,6 +102,9 @@ struct usbtmc_device_data {
u16iin_wMaxPacketSize;
atomic_t srq_asserted;
+ /* coalesced usb488_caps from usbtmc_dev_capabilities */
+ u8 usb488_caps;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -992,6 +995,7 @@ static int get_capabilities(struct usbtmc_device_data *data)
data->capabilities.device_capabilities = buffer[5];
data->capabilities.usb488_interface_capabilities = buffer[14];
data->capabilities.usb488_device_capabilities = buffer[15];
+ data->usb488_caps = (buffer[14] & 0x07) | ((buffer[15] & 0x0f) << 4);
rv = 0;
err_out:
@@ -1167,6 +1171,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+ case USBTMC488_IOCTL_GET_CAPS:
+ retval = copy_to_user((void __user *)arg,
+ >usb488_caps,
+ sizeof(data->usb488_caps));
+ if (retval)
+ retval = -EFAULT;
+ break;
+
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 7e5ced8..1dc3af1 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -2,12 +2,14 @@
* Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
* Copyright (C) 2008 Novell, Inc.
* Copyright (C) 2008 Greg Kroah-Hartman <gre...@suse.de>
+ * Copyright (C) 2015 Dave Penkler <dpenk...@gmail.com>
*
* This file holds USB constants defined by the USB Device Class
- * Definition for Test and Measurement devices published by the USB-IF.
+ * and USB488 Subclass Definitions for Test and Measurement devices
+ * published by the USB-IF.
*
- * It also has the ioctl definitions for the usbtmc kernel driver that
- * userspace needs to know about.
+ * It also has the ioctl and capability definitions for the
+ * usbtmc kernel driver that userspace needs to know about.
*/
#ifndef __LINUX_USB_TMC_H
@@ -40,6 +42,19 @@
#define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4)
#define USBTMC_IOCTL_CLEAR_OUT_HALT_IO(USBTMC_IOC_NR, 6)
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
+#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+/* Driver encoded usb488 capabilities */
+#define USBTMC488_CAPABILITY_TRIGGER 1
+#define USBTMC488_CAPABILITY_SIMPLE 2
+#define USBTMC488_CAPABILITY_REN_CONTROL 2
+#define USBTMC488_CAPABILITY_GOTO_LOCAL 2
+#define USBTMC488_CAPABILITY_LOCAL_LOCKOUT 2
+#define USBTMC488_CAPABILITY_488_DOT_2 4
+#define USBTMC488_CAPABILITY_DT1 16
+#define USBTMC488_CAPABILITY_RL1 32
+#define USBTMC488_CAPABILITY_SR1 64
+#define USBTMC488_CAPABILITY_FULL_SCPI 128
+
#endif
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 5/5] Add ioctls to enable and disable local controls on an instrument
These ioctls provide support for the USBTMC-USB488 control requests
for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 71
include/uapi/linux/usb/tmc.h | 6
2 files changed, 77 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 3a3264c..f04a086 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -473,6 +473,62 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
return rv;
}
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+ unsigned long arg,
+ unsigned int cmd)
+{
+ struct device *dev = >intf->dev;
+ unsigned int val;
+ u8 *buffer;
+ u16 wValue;
+ int rv;
+
+ if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+ return -EINVAL;
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+ rv = copy_from_user(, (void __user *)arg, sizeof(val));
+ if (rv) {
+ rv = -EFAULT;
+ goto exit;
+ }
+ wValue = val ? 1 : 0;
+ } else {
+ wValue = 0;
+ }
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ cmd,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ wValue,
+ data->ifnum,
+ buffer, 0x01, USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+ goto exit;
+ } else if (rv != 1) {
+ dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "simple control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+ rv = 0;
+
+ exit:
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1182,6 +1238,21 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
+
+ case USBTMC488_IOCTL_REN_CONTROL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_REN_CONTROL);
+ break;
+
+ case USBTMC488_IOCTL_GOTO_LOCAL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_GOTO_LOCAL);
+ break;
+
+ case USBTMC488_IOCTL_LOCAL_LOCKOUT:
+ retval = usbtmc488_ioctl_simple(data, arg,
+
USBTMC488_REQUEST_LOCAL_LOCKOUT);
+ break;
}
skip_io_on_zombie:
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 1dc3af1..0d852c9 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -33,6 +33,9 @@
#define USBTMC_REQUEST_GET_CAPABILITIES7
#define USBTMC_REQUEST_INDICATOR_PULSE 64
#define USBTMC488_REQUEST_READ_STATUS_BYTE 128
+#define USBTMC488_REQUEST_REN_CONTROL 160
+#define USBTMC488_REQUEST_GOTO_LOCAL 161
+#define USBTMC488_REQUEST_LOCAL_LOCKOUT162
/* Request values for USBTMC driver's ioctl entry point */
#define USBTMC_IOC_NR 91
@@ -44,6 +47,9 @@
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+#define USBTMC488_IOCTL_REN_CONTROL_IOW(USBTMC_IOC_NR, 19, unsigned char)
+#define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20)
+#define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21)
/* Driver encoded usb488 capabilities */
#define USBTMC488_CAPABILITY_TRIGGER 1
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec
Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) and SRQ notifications with fasync (2/5) and poll/select (3/5) in order to be able to synchronize with variable duration instrument operations. Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and LOCAL_LOCKOUT. (4/5) Add convenience ioctl to return all device capabilities (5/5) PATCH Changelog: v6 - Remove more excess newlines Rearrange declaration blocks aesthetically Remove __func__ parameter from dev_xxx calls Simplify tests for interrupt-in notifications Propagate return code from usb_submit_urb() v5 - Remove excess newlines and parens - Move dev variable initialisations to declaration - Add comment on interrupt btag wrap - simplify test in usbtmc_free_int() v4 - Remove excess newlines and parens - simplify some expressions v3 - Split into multiple patches as per gregkh request V2 - Fix V1 bug: not waking sleepers on disconnect. - Correct sparse warnings. V1 - Original patch Testing: All functions tested ok on an USBTMC-USB488 compliant oscilloscope Dave Penkler (5): Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Add support for USBTMC USB488 SRQ notification with fasync Add support for receiving USBTMC USB488 SRQ notifications via poll/select Add ioctl to retrieve USBTMC-USB488 capabilities Add ioctls to enable and disable local controls on an instrument drivers/usb/class/usbtmc.c | 330 +++ include/uapi/linux/usb/tmc.h | 29 +++- 2 files changed, 356 insertions(+), 3 deletions(-) -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 2/5] Add support for USBTMC USB488 SRQ notification with fasync
Background:
By configuring an instrument's event status register various
conditions can be reported via an SRQ notification. This complements
the synchronous polling approach using the READ_STATUS_BYTE ioctl
with an asynchronous notification.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 89456df9..7bfd6ec 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -99,6 +99,7 @@ struct usbtmc_device_data {
intiin_interval;
struct urb*iin_urb;
u16iin_wMaxPacketSize;
+ atomic_t srq_asserted;
u8 rigol_quirk;
@@ -113,6 +114,7 @@ struct usbtmc_device_data {
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
wait_queue_head_t waitq;
+ struct fasync_struct *fasync;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -404,6 +406,9 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
atomic_set(>iin_data_valid, 0);
+ /* must issue read_stb before using poll or select */
+ atomic_set(>srq_asserted, 0);
+
rv = usb_control_msg(data->usb_dev,
usb_rcvctrlpipe(data->usb_dev, 0),
USBTMC488_REQUEST_READ_STATUS_BYTE,
@@ -1171,6 +1176,13 @@ skip_io_on_zombie:
return retval;
}
+static int usbtmc_fasync(int fd, struct file *file, int on)
+{
+ struct usbtmc_device_data *data = file->private_data;
+
+ return fasync_helper(fd, file, on, >fasync);
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1178,6 +1190,7 @@ static const struct file_operations fops = {
.open = usbtmc_open,
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
+ .fasync = usbtmc_fasync,
.llseek = default_llseek,
};
@@ -1207,6 +1220,16 @@ static void usbtmc_interrupt(struct urb *urb)
wake_up_interruptible(>waitq);
goto exit;
}
+ /* check for SRQ notification */
+ if (data->iin_buffer[0] == 0x81) {
+ if (data->fasync)
+ kill_fasync(>fasync,
+ SIGIO, POLL_IN);
+
+ atomic_set(>srq_asserted, 1);
+ wake_up_interruptible(>waitq);
+ goto exit;
+ }
dev_warn(dev, "invalid notification: %x\n",
data->iin_buffer[0]);
break;
case -EOVERFLOW:
@@ -1262,6 +1285,7 @@ static int usbtmc_probe(struct usb_interface *intf,
mutex_init(>io_mutex);
init_waitqueue_head(>waitq);
atomic_set(>iin_data_valid, 0);
+ atomic_set(>srq_asserted, 0);
data->zombie = 0;
/* Determine if it is a Rigol or not */
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 3/5] Add support for receiving USBTMC USB488 SRQ notifications via poll/select
Background:
In many situations operations on multiple instruments need to be
synchronized. poll/select provide a convenient way of waiting on a
number of different instruments and other peripherals
simultaneously.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7bfd6ec..3b85ef5 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1183,6 +1184,27 @@ static int usbtmc_fasync(int fd, struct file *file, int
on)
return fasync_helper(fd, file, on, >fasync);
}
+static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
+{
+ struct usbtmc_device_data *data = file->private_data;
+ unsigned int mask;
+
+ mutex_lock(>io_mutex);
+
+ if (data->zombie) {
+ mask = POLLHUP | POLLERR;
+ goto no_poll;
+ }
+
+ poll_wait(file, >waitq, wait);
+
+ mask = (atomic_read(>srq_asserted)) ? POLLIN | POLLRDNORM : 0;
+
+no_poll:
+ mutex_unlock(>io_mutex);
+ return mask;
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1191,6 +1213,7 @@ static const struct file_operations fops = {
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
.fasync = usbtmc_fasync,
+ .poll = usbtmc_poll,
.llseek = default_llseek,
};
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler <dpenk...@gmail.com> wrote:
> > On Sun, Nov 22, 2015 at 12:32:41PM +0200, Andy Shevchenko wrote:
> >> On Sun, Nov 22, 2015 at 11:19 AM, Dave Penkler <dpenk...@gmail.com> wrote:
> >> > On Wed, Nov 18, 2015 at 11:55:27AM +0200, Andy Shevchenko wrote:
> >> >> On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <dpenk...@gmail.com>
> >> >> wrote:
> >>
> >> >> > + switch (status) {
> >> >> > + case 0: /* SUCCESS */
> >> >> > + if (data->iin_buffer[0] & 0x80) {
> >> >> > + /* check for valid STB notification */
> >> >> > + if ((data->iin_buffer[0] & 0x7f) > 1) {
> >> >>
> >> >> Despite your answer to my comment code is quite understandable even
> >> >> with & 0x7e.
> >> >> You already put comment line about this, you may add that you validate
> >> >> the value to be 127 >= value >= 2.
> >> >>
> >> >
> >> > Yes it is quite understandable but it is less clear. I repeat my comment
> >> > here:
> >> > When reading the spec and the code it is more obvious that here
> >> > we are testing for the value in bits D6..D0 to be a valid iin_bTag
> >> > return.
> >> > (See Table 7 in the USBTMC-USB488 spec.)
> >> >
> >> > What is your motivation for
> >> >
> >> > if (data->iin_buffer[0] & 0x7e)
> >> >
> >> > ?
> >>
> >> In non-optimized variant it will certainly generate less code. You may
> >> have check assembly code with -O2 and compare. I don't know if
> >> compiler is clever enough to do the same by itself.
> >>
> >
> > I tested out both variants, and the explicit test is actually faster on by
> > box:
> >
> > $ cat tp.c
> > #include
> > #include
> > #define xstr(s) str(s)
> > #define str(s) #s
> > main() {
> > unsigned int v,s=0;
> > struct recs {
> > unsigned char *iin_buffer;
> > } rec;
> > struct recs *data =
> > data->iin_buffer = (unsigned char *) malloc(8);
> > for (v=1;v;v++) {
>
> > data->iin_buffer[0] = v & 0x7f;
>
> This line makes test fragile.
You are right, ignore this test.
snip
> Can you, please, check the assembly code in the real driver?
Below are the generated assembly code fragments using the standard
kernel makefile flags. The opcodes for the relevant instructions
are in capital letters. Comments added to show correspondence with C code.
Note that it is the combination of the two tests that must be considered:
6 instructions for the 0x7e version and 5 for the original.
Performance is the same so I guess we can stick with the original ?
Assembly for (data->iin_buffer[0] & 0x7e) version
.L258:
TESTB $126, %dl # if (data->iin_buffer[0] & 0x7e) goto moveit
JNE .L260
MOVL%edx, %eax # if ((data->iin_buffer[0] & 0x7f) == 1) goto tfasync
ANDL$127, %eax
CMPB$1, %al
JNE .L234 # else goto .L234
tfasync cmpq$0, 168(%r12)
je .L237
leaq168(%r12), %rdi
movl$131073, %edx
movl$29, %esi
callkill_fasync
.L237:
movl$1, 84(%r12)
.L255:
[snip]
.L260:
moveit movb%dl, 35(%r12)
movzbl 1(%rax), %eax
movl$1, 56(%r12)
movb%al, 36(%r12)
jmp .L255
Assembly for ((data->iin_buffer[0] & 0x7f) > 1) version
.L258:
MOVL%edx, %ecx # if ((data->iin_buffer[0] & 0x7f) > 1) goto moveit
ANDL$127, %ecx
CMPB$1, %cl
JBE .L235# else goto .L235
moveit movb%dl, 35(%r12)
movzbl 1(%rax), %eax
movl$1, 56(%r12)
movb%al, 36(%r12)
.L255:
[snip]
.L235:
JNE .L234# if ((data->iin_buffer[0] & 0x7f) == 1) goto tfasync
# else goto .L234
tfasync cmpq$0, 168(%r12)
je .L237
leaq168(%r12), %rdi
movl$131073, %edx
movl$29, %esi
callkill_fasync
.L237:
movl$1, 84(%r12)
jmp .L255
> I can't do this right now, maybe tomorrow I will have few minutes to check
> that.
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
On Sat, Nov 28, 2015 at 04:57:47PM +0200, Andy Shevchenko wrote:
> On Sat, Nov 28, 2015 at 1:55 PM, Dave Penkler <dpenk...@gmail.com> wrote:
> > On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
> >> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler <dpenk...@gmail.com> wrote:
> >> > On Sun, Nov 22, 2015 at 12:32:41PM +0200, Andy Shevchenko wrote:
> >> >> On Sun, Nov 22, 2015 at 11:19 AM, Dave Penkler <dpenk...@gmail.com>
> >> >> wrote:
> >> >> > On Wed, Nov 18, 2015 at 11:55:27AM +0200, Andy Shevchenko wrote:
> >> >> >> On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <dpenk...@gmail.com>
> >> >> >> wrote:
>
> Thank you for an update!
>
> >> >> >> > + switch (status) {
> >> >> >> > + case 0: /* SUCCESS */
> >> >> >> > + if (data->iin_buffer[0] & 0x80) {
> >> >> >> > + /* check for valid STB notification */
> >> >> >> > + if ((data->iin_buffer[0] & 0x7f) > 1) {
>
> How can I miss that there are two conditionals in a sequence and
> moreover for the same data?!
Sorry, my fault, it is the combination of patch 1 and 2
> That might explain the optimization done by compiler.
>
> So, could it be transformed to simple one condition
>if (data->iin_buffer[0] > 0x81 /* 129 */) {
> ?
OK so now for patch 1 and 2 we have:
switch (status) {
case 0: /* SUCCESS */
/* PATCH 1 check for valid STB notification */
if (data->iin_buffer[0] > 0x81) {
data->bNotify1 = data->iin_buffer[0];
data->bNotify2 = data->iin_buffer[1];
atomic_set(>iin_data_valid, 1);
wake_up_interruptible(>waitq);
goto exit;
}
/* PATCH 2 check for SRQ notification */
if (data->iin_buffer[0] == 0x81) {
if (data->fasync)
kill_fasync(>fasync,
SIGIO, POLL_IN);
atomic_set(>srq_asserted, 1);
wake_up_interruptible(>waitq);
goto exit;
}
I'll push a new set if you are OK with this.
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument
On Sun, Nov 22, 2015 at 12:36:53PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 22, 2015 at 10:51 AM, Dave Penkler <dpenk...@gmail.com> wrote:
> > On Wed, Nov 18, 2015 at 11:41:30AM +0200, Andy Shevchenko wrote:
> >> On Wed, Nov 18, 2015 at 10:38 AM, Dave Penkler <dpenk...@gmail.com> wrote:
>
>
> >> > + if (rv < 0) {
> >> > + dev_err(dev, "simple usb_control_msg failed %d\n", rv);
> >> > + goto exit;
> >> > + } else if (rv != 1) {
> >> > + dev_warn(dev, "simple usb_control_msg returned %d\n",
> >> > rv);
> >>
> >> Actually here what king of results could be? 0? 2+? In all cases of
> >> error you have to provide an error code.
> >>
> >
> > We seem to be going round in circles here, last time you suggested to
> > propagate the return value.
>
> You didn't pay much attention to where I put my comment. You have few
> branches depending on return value
>
> 1) negative, apparently an error code, should be propagated if nothing
> specific to framework;
> 2) zero, what does it means?
Zero bytes transferred -> error
> 3) one, seems the expected result when success, so, error code should be 0;
Which it is when we drop through
> 4) two, three, ??? non-negative numbers,see 2).
>
> For my understanding 2) and 4) have to return what you initially had -EIO.
>
> > The non-negative return is the number of bytes
> > transferred which should be 1 unless there is some usb implementation
> > flakiness happening. So I will go back to returning -EIO.
>
> Yes, in *this* branch.
>
OK so now I have the same as in v4 again:
if (rv < 0) {
dev_err(dev, "simple usb_control_msg failed %d\n", rv);
goto exit;
} else if (rv != 1) {
dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
rv = -EIO;
goto exit;
}
> >
> >> > + goto exit;
> >> > + }
> >> > +
> >> > + if (buffer[0] != USBTMC_STATUS_SUCCESS) {
> >> > + dev_err(dev, "simple control status returned %x\n",
> >> > buffer[0]);
> >> > + rv = -EIO;
> >> > + goto exit;
> >> > + }
> >> > + rv = 0;
> >> > +
> >> > + exit:
> >> > + kfree(buffer);
> >> > + return rv;
> >> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
On Sun, Nov 22, 2015 at 12:32:41PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 22, 2015 at 11:19 AM, Dave Penkler <dpenk...@gmail.com> wrote:
> > On Wed, Nov 18, 2015 at 11:55:27AM +0200, Andy Shevchenko wrote:
> >> On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <dpenk...@gmail.com> wrote:
>
> >> > + switch (status) {
> >> > + case 0: /* SUCCESS */
> >> > + if (data->iin_buffer[0] & 0x80) {
> >> > + /* check for valid STB notification */
> >> > + if ((data->iin_buffer[0] & 0x7f) > 1) {
> >>
> >> Despite your answer to my comment code is quite understandable even with &
> >> 0x7e.
> >> You already put comment line about this, you may add that you validate
> >> the value to be 127 >= value >= 2.
> >>
> >
> > Yes it is quite understandable but it is less clear. I repeat my comment
> > here:
> > When reading the spec and the code it is more obvious that here
> > we are testing for the value in bits D6..D0 to be a valid iin_bTag return.
> > (See Table 7 in the USBTMC-USB488 spec.)
> >
> > What is your motivation for
> >
> > if (data->iin_buffer[0] & 0x7e)
> >
> > ?
>
> In non-optimized variant it will certainly generate less code. You may
> have check assembly code with -O2 and compare. I don't know if
> compiler is clever enough to do the same by itself.
>
I tested out both variants, and the explicit test is actually faster on by box:
$ cat tp.c
#include
#include
#define xstr(s) str(s)
#define str(s) #s
main() {
unsigned int v,s=0;
struct recs {
unsigned char *iin_buffer;
} rec;
struct recs *data =
data->iin_buffer = (unsigned char *) malloc(8);
for (v=1;v;v++) {
data->iin_buffer[0] = v & 0x7f;
if (TEST)
s++;
}
printf("%s %x\n",xstr(TEST),s);
}
$ cc -O2 tp.c -DTEST='data->iin_buffer[0] & 0x7e'
$ time ./a.out
data->iin_buffer[0] & 0x7e fc00
real0m3.927s
user0m3.920s
sys 0m0.000s
$ time ./a.out
data->iin_buffer[0] & 0x7e fc00
real0m3.925s
user0m3.920s
sys 0m0.000s
$ cc -O2 tp.c -DTEST='(data->iin_buffer[0] & 0x7f) > 1'
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc00
real0m2.638s
user0m2.610s
sys 0m0.000s
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc00
real0m2.648s
user0m2.620s
sys 0m0.000s
> >> > + /* urb terminated, clean up */
> >> > + dev_dbg(>intf->dev,
> >> > + "%s - urb terminated, status: %d\n",
> >> > + __func__, status);
> >>
> >> No need to print function here explicitly. Check Dynamic Debug framework.
> >
> > I am not using dynamic debug but when I enable static debug I get:
> >
> > [ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1
> >
> > on the console log for
> >
> > dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
> > data->iin_ep_present);
> >
> > So if I don't print the function it does not appear on the log.
>
> Whatever maintainers prefer, though I think there are quite rare cases
> in USB when someone needs static debug. I'm pretty sure most of the
> developers all in favour of dynamic debug.
>
I am happy to remove the func
> >> > retcode = sysfs_create_group(>dev.kobj, _attr_grp);
> >> >
> >> > retcode = usb_register_dev(intf, _class);
> >>
> >> Hmm??? Unrelated to this patch, but notice that retcode is overridden here.
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument
On Wed, Nov 18, 2015 at 11:41:30AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 18, 2015 at 10:38 AM, Dave Penkler <dpenk...@gmail.com> wrote:
> > These ioctls provide support for the USBTMC-USB488 control requests
> > for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
>
> Couple of comments below.
>
> > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > index 2358991..d416a5f 100644
> > --- a/drivers/usb/class/usbtmc.c
> > +++ b/drivers/usb/class/usbtmc.c
> > @@ -476,6 +476,62 @@ static int usbtmc488_ioctl_read_stb(struct
> > usbtmc_device_data *data,
snip
>
> > + if (rv < 0) {
> > + dev_err(dev, "simple usb_control_msg failed %d\n", rv);
> > + goto exit;
> > + } else if (rv != 1) {
> > + dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
>
> Actually here what king of results could be? 0? 2+? In all cases of
> error you have to provide an error code.
>
We seem to be going round in circles here, last time you suggested to
propagate the return value. The non-negative return is the number of bytes
transferred which should be 1 unless there is some usb implementation
flakiness happening. So I will go back to returning -EIO.
> > + goto exit;
> > + }
> > +
> > + if (buffer[0] != USBTMC_STATUS_SUCCESS) {
> > + dev_err(dev, "simple control status returned %x\n",
> > buffer[0]);
> > + rv = -EIO;
> > + goto exit;
> > + }
> > + rv = 0;
> > +
> > + exit:
> > + kfree(buffer);
> > + return rv;
> > +}
> > +
>
> --
> With Best Regards,
> Andy Shevchenko
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
On Wed, Nov 18, 2015 at 11:55:27AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <dpenk...@gmail.com> wrote:
> > Background:
> > When performing a read on an instrument that is executing a function
> > that runs longer than the USB timeout the instrument may hang and
> > require a device reset to recover. The READ_STATUS_BYTE operation
> > always returns even when the instrument is busy permitting to poll
> > for the appropriate condition. This capability is referred to in
> > instrument application notes on synchronizing acquisitions for other
> > platforms.
> >
>
> First of all, please be patient and do not send patches immediately
> when you answered to someone's review. It increases redundant noise
> and burden on reviewer. Wait at least for 24h especially if there are
> topics still to discuss.
>
OK, apologies.
snip
> > +
> > + switch (status) {
> > + case 0: /* SUCCESS */
> > + if (data->iin_buffer[0] & 0x80) {
> > + /* check for valid STB notification */
> > + if ((data->iin_buffer[0] & 0x7f) > 1) {
>
> Despite your answer to my comment code is quite understandable even with &
> 0x7e.
> You already put comment line about this, you may add that you validate
> the value to be 127 >= value >= 2.
>
Yes it is quite understandable but it is less clear. I repeat my comment here:
When reading the spec and the code it is more obvious that here
we are testing for the value in bits D6..D0 to be a valid iin_bTag return.
(See Table 7 in the USBTMC-USB488 spec.)
What is your motivation for
if (data->iin_buffer[0] & 0x7e)
?
> > + data->bNotify1 = data->iin_buffer[0];
> > + data->bNotify2 = data->iin_buffer[1];
> > + atomic_set(>iin_data_valid, 1);
> > + wake_up_interruptible(>waitq);
> > + goto exit;
> > + }
> > + }
snip
> > + /* urb terminated, clean up */
> > + dev_dbg(>intf->dev,
> > + "%s - urb terminated, status: %d\n",
> > + __func__, status);
>
> No need to print function here explicitly. Check Dynamic Debug framework.
I am not using dynamic debug but when I enable static debug I get:
[ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1
on the console log for
dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
data->iin_ep_present);
So if I don't print the function it does not appear on the log.
>
> > + return;
> > + default:
> > + dev_err(>intf->dev,
> > + "%s - unknown status received: %d\n",
> > + __func__, status);
> > + }
> > +exit:
> > + rv = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (rv) {
> > + dev_err(>intf->dev, "%s - usb_submit_urb failed:
> > %d\n",
> > + __func__, rv);
> > + }
> > +}
snip
> > +
> > + /* fill interrupt urb */
> > + usb_fill_int_urb(data->iin_urb, data->usb_dev,
> > + usb_rcvintpipe(data->usb_dev, data->iin_ep),
> > + data->iin_buffer, data->iin_wMaxPacketSize,
> > + usbtmc_interrupt,
> > + data, data->iin_interval);
> > +
> > + if (usb_submit_urb(data->iin_urb, GFP_KERNEL)) {
>
> Does it return a proper error code?
>
Yes, will propagate it.
> > + retcode = -EIO;
> > + dev_err(>dev, "Failed to submit iin_urb\n");
> > + goto error_register;
> > + }
> > + }
> > +
>
> > retcode = sysfs_create_group(>dev.kobj, _attr_grp);
> >
> > retcode = usb_register_dev(intf, _class);
>
> Hmm??? Unrelated to this patch, but notice that retcode is overridden here.
>
>
> > @@ -1185,6 +1395,7 @@ static int usbtmc_probe(struct usb_interface *intf,
> > error_register:
> > sysfs_remove_group(>dev.kobj, _attr_grp);
> > sysfs_remove_group(>dev.kobj, _attr_grp);
> > + usbtmc_free_int(data);
> > kref_put(>kref, usbtmc_delete);
> > return retcode;
> > }
>
>
> --
> With Best Regards,
> Andy Shevchenko
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/5] Add ioctls to enable and disable local controls on an instrument
On Sun, Nov 15, 2015 at 10:10:35PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 15, 2015 at 8:40 PM, Dave Penkler <dpenk...@gmail.com> wrote:
> > These ioctls provide support for the USBTMC-USB488 control requests
> > for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
> >
> > +
> > + dev = >intf->dev;
>
> Could be assigned above.
>
ok
> > + wValue = (val) ? 1 : 0;
>
> Redundant parens.
>
ok
> > + rv = -EIO;
>
> Hmm??? Does usb_control_msg() return a proper error code? If it does,
> perhaps better to propagate it.
>
You are right.
> > + rv = -EIO;
> > + goto exit;
>
> > + } else {
>
> Redundant else.
>
OK
>
Thanks,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
Hi Andy,
On Sun, Nov 15, 2015 at 10:04:10PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 15, 2015 at 8:39 PM, Dave Penkler <dpenk...@gmail.com> wrote:
snip
> > +
>
> Redundant empty line.
>
ok
>
> > + data->iin_bTag = 2;
>
> Hmm??? Why 2?
> A-ha, below I found a comment. Something might be good to have here as well.
>
Added comment
> > +
>
> Redundant empty line.
>
ok
> > +
> > + if (data->iin_buffer[0] & 0x80) {
> > + /* check for valid STB notification */
> > + if ((data->iin_buffer[0] & 0x7f) > 1) {
>
> It's the same as
> if (data->iin_buffer[0] & 0x7e) {
>
Yes but when reading the spec and the code it is more obvious that here
we are testing for the value in bits D6..D0 to be a valid iin_bTag return.
(See Table 7 in the USBTMC-USB488 spec.)
> > + dev_dbg(>intf->dev,
> > + "%s - urb terminated, status: %d\n",
>
> I heard that dynamic debug adds function name.
>
> > + __func__, status);
I checked in device.h and could not find any trace of it. I'm on 4.4.0-rc1.
> > +static void usbtmc_free_int(struct usbtmc_device_data *data)
> > +{
> > + if (data->iin_ep_present) {
> > + if (data->iin_urb) {
>
> Why not
>
> if (!data->iin_ep_present || !data->iin_urb)
> return;
>
> ?
>
OK
Thanks,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/5] Add support for receiving USBTMC USB488 SRQ notifications via poll/select
Background:
In many situations operations on multiple instruments need to be
synchronized. poll/select provide a convenient way of waiting on a
number of different instruments and other peripherals
simultaneously.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index a95a63d..6294a3c 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1186,6 +1187,27 @@ static int usbtmc_fasync(int fd, struct file *file, int
on)
return fasync_helper(fd, file, on, >fasync);
}
+static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
+{
+ struct usbtmc_device_data *data = file->private_data;
+ unsigned int mask;
+
+ mutex_lock(>io_mutex);
+
+ if (data->zombie) {
+ mask = POLLHUP | POLLERR;
+ goto no_poll;
+ }
+
+ poll_wait(file, >waitq, wait);
+
+ mask = (atomic_read(>srq_asserted)) ? POLLIN | POLLRDNORM : 0;
+
+no_poll:
+ mutex_unlock(>io_mutex);
+ return mask;
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1194,6 +1216,7 @@ static const struct file_operations fops = {
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
.fasync = usbtmc_fasync,
+ .poll = usbtmc_poll,
.llseek = default_llseek,
};
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 4/5] Add ioctl to retrieve USBTMC-USB488 capabilities
This is a convenience function to obtain an instrument's
capabilities from its file descriptor without having to access sysfs
from the user program.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 12
include/uapi/linux/usb/tmc.h | 21 ++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6294a3c..2358991 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -102,6 +102,9 @@ struct usbtmc_device_data {
u16iin_wMaxPacketSize;
atomic_t srq_asserted;
+ /* coalesced usb488_caps from usbtmc_dev_capabilities */
+ u8 usb488_caps;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -995,6 +998,7 @@ static int get_capabilities(struct usbtmc_device_data *data)
data->capabilities.device_capabilities = buffer[5];
data->capabilities.usb488_interface_capabilities = buffer[14];
data->capabilities.usb488_device_capabilities = buffer[15];
+ data->usb488_caps = (buffer[14] & 0x07) | ((buffer[15] & 0x0f) << 4);
rv = 0;
err_out:
@@ -1170,6 +1174,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+ case USBTMC488_IOCTL_GET_CAPS:
+ retval = copy_to_user((void __user *)arg,
+ >usb488_caps,
+ sizeof(data->usb488_caps));
+ if (retval)
+ retval = -EFAULT;
+ break;
+
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 7e5ced8..1dc3af1 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -2,12 +2,14 @@
* Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
* Copyright (C) 2008 Novell, Inc.
* Copyright (C) 2008 Greg Kroah-Hartman <gre...@suse.de>
+ * Copyright (C) 2015 Dave Penkler <dpenk...@gmail.com>
*
* This file holds USB constants defined by the USB Device Class
- * Definition for Test and Measurement devices published by the USB-IF.
+ * and USB488 Subclass Definitions for Test and Measurement devices
+ * published by the USB-IF.
*
- * It also has the ioctl definitions for the usbtmc kernel driver that
- * userspace needs to know about.
+ * It also has the ioctl and capability definitions for the
+ * usbtmc kernel driver that userspace needs to know about.
*/
#ifndef __LINUX_USB_TMC_H
@@ -40,6 +42,19 @@
#define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4)
#define USBTMC_IOCTL_CLEAR_OUT_HALT_IO(USBTMC_IOC_NR, 6)
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
+#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+/* Driver encoded usb488 capabilities */
+#define USBTMC488_CAPABILITY_TRIGGER 1
+#define USBTMC488_CAPABILITY_SIMPLE 2
+#define USBTMC488_CAPABILITY_REN_CONTROL 2
+#define USBTMC488_CAPABILITY_GOTO_LOCAL 2
+#define USBTMC488_CAPABILITY_LOCAL_LOCKOUT 2
+#define USBTMC488_CAPABILITY_488_DOT_2 4
+#define USBTMC488_CAPABILITY_DT1 16
+#define USBTMC488_CAPABILITY_RL1 32
+#define USBTMC488_CAPABILITY_SR1 64
+#define USBTMC488_CAPABILITY_FULL_SCPI 128
+
#endif
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec
Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) and SRQ notifications with fasync (2/5) and poll/select (3/5) in order to be able to synchronize with variable duration instrument operations. Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and LOCAL_LOCKOUT. (4/5) Add convenience ioctl to return all device capabilities (5/5) PATCH Changelog: v5 - Remove excess newlines and parens - Move dev variable initialisations to declaration - Add comment on interrupt btag wrap - simplify test in usbtmc_free_int() v4 - Remove excess newlines and parens - simplify some expressions v3 - Split into multiple patches as per gregkh request V2 - Fix V1 bug: not waking sleepers on disconnect. - Correct sparse warnings. V1 - Original patch Testing: All functions tested ok on an USBTMC-USB488 compliant oscilloscope Dave Penkler (5): Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Add support for USBTMC USB488 SRQ notification with fasync Add support for receiving USBTMC USB488 SRQ notifications via poll/select Add ioctl to retrieve USBTMC-USB488 capabilities Add ioctls to enable and disable local controls on an instrument drivers/usb/class/usbtmc.c | 342 +++ include/uapi/linux/usb/tmc.h | 29 +++- 2 files changed, 368 insertions(+), 3 deletions(-) -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/5] Add support for USBTMC USB488 SRQ notification with fasync
Background:
By configuring an instrument's event status register various
conditions can be reported via an SRQ notification. This complements
the synchronous polling approach using the READ_STATUS_BYTE ioctl
with an asynchronous notification.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 60a71cb..a95a63d 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -99,6 +99,7 @@ struct usbtmc_device_data {
intiin_interval;
struct urb*iin_urb;
u16iin_wMaxPacketSize;
+ atomic_t srq_asserted;
u8 rigol_quirk;
@@ -113,6 +114,7 @@ struct usbtmc_device_data {
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
wait_queue_head_t waitq;
+ struct fasync_struct *fasync;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -404,6 +406,9 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
atomic_set(>iin_data_valid, 0);
+ /* must issue read_stb before using poll or select */
+ atomic_set(>srq_asserted, 0);
+
rv = usb_control_msg(data->usb_dev,
usb_rcvctrlpipe(data->usb_dev, 0),
USBTMC488_REQUEST_READ_STATUS_BYTE,
@@ -1174,6 +1179,13 @@ skip_io_on_zombie:
return retval;
}
+static int usbtmc_fasync(int fd, struct file *file, int on)
+{
+ struct usbtmc_device_data *data = file->private_data;
+
+ return fasync_helper(fd, file, on, >fasync);
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1181,6 +1193,7 @@ static const struct file_operations fops = {
.open = usbtmc_open,
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
+ .fasync = usbtmc_fasync,
.llseek = default_llseek,
};
@@ -1210,6 +1223,16 @@ static void usbtmc_interrupt(struct urb *urb)
wake_up_interruptible(>waitq);
goto exit;
}
+ /* check for SRQ notification */
+ if ((data->iin_buffer[0] & 0x7f) == 1) {
+ if (data->fasync)
+ kill_fasync(>fasync,
+ SIGIO, POLL_IN);
+
+ atomic_set(>srq_asserted, 1);
+ wake_up_interruptible(>waitq);
+ goto exit;
+ }
}
dev_warn(>intf->dev, "invalid notification: %x\n",
data->iin_buffer[0]);
@@ -1274,6 +1297,7 @@ static int usbtmc_probe(struct usb_interface *intf,
mutex_init(>io_mutex);
init_waitqueue_head(>waitq);
atomic_set(>iin_data_valid, 0);
+ atomic_set(>srq_asserted, 0);
data->zombie = 0;
/* Determine if it is a Rigol or not */
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
Background:
When performing a read on an instrument that is executing a function
that runs longer than the USB timeout the instrument may hang and
require a device reset to recover. The READ_STATUS_BYTE operation
always returns even when the instrument is busy permitting to poll
for the appropriate condition. This capability is referred to in
instrument application notes on synchronizing acquisitions for other
platforms.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 212 +++
include/uapi/linux/usb/tmc.h | 2 +
2 files changed, 214 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..60a71cb 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
+ /* data for interrupt in endpoint handling */
+ u8 bNotify1;
+ u8 bNotify2;
+ u16ifnum;
+ u8 iin_bTag;
+ u8*iin_buffer;
+ atomic_t iin_data_valid;
+ unsigned int iin_ep;
+ intiin_ep_present;
+ intiin_interval;
+ struct urb*iin_urb;
+ u16iin_wMaxPacketSize;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -99,6 +112,7 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities capabilities;
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
+ wait_queue_head_t waitq;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -373,6 +387,86 @@ exit:
return rv;
}
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+ unsigned long arg)
+{
+ u8 *buffer;
+ struct device *dev = >intf->dev;
+ int rv;
+ u8 tag, stb;
+
+ dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+ data->iin_ep_present);
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ atomic_set(>iin_data_valid, 0);
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC488_REQUEST_READ_STATUS_BYTE,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ data->iin_bTag,
+ data->ifnum,
+ buffer, 0x03, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (data->iin_ep_present) {
+ rv = wait_event_interruptible_timeout(
+ data->waitq,
+ atomic_read(>iin_data_valid) != 0,
+ USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_dbg(dev, "wait interrupted %d\n", rv);
+ goto exit;
+ }
+
+ if (rv == 0) {
+ dev_dbg(dev, "wait timed out\n");
+ rv = -ETIME;
+ goto exit;
+ }
+
+ tag = data->bNotify1 & 0x7f;
+
+ if (tag != data->iin_bTag) {
+ dev_err(dev, "expected bTag %x got %x\n",
+ data->iin_bTag, tag);
+ }
+
+ stb = data->bNotify2;
+ } else {
+ stb = buffer[2];
+ }
+
+ rv = copy_to_user((void __user *)arg, , sizeof(stb));
+ if (rv)
+ rv = -EFAULT;
+
+ exit:
+ /* bump interrupt bTag */
+ data->iin_bTag += 1;
+ if (data->iin_bTag > 127)
+ /* 1 is for SRQ see USBTMC-USB488 subclass specification 4.3.1*/
+ data->iin_bTag = 2;
+
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1069,6 +1163,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC_IOCTL_ABORT_BULK_IN:
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+
+ case USBTMC488_IOCTL_READ_STB:
+ retval = usbtmc488_ioctl_read_stb(data, arg);
+ break;
}
skip_io_on_zombie:
@@ -1092,6 +1190,66 @@ static st
[PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument
These ioctls provide support for the USBTMC-USB488 control requests
for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 71
include/uapi/linux/usb/tmc.h | 6
2 files changed, 77 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 2358991..d416a5f 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -476,6 +476,62 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
return rv;
}
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+ unsigned long arg,
+ unsigned int cmd)
+{
+ u8 *buffer;
+ struct device *dev = >intf->dev;
+ int rv;
+ unsigned int val;
+ u16 wValue;
+
+ if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+ return -EINVAL;
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+ rv = copy_from_user(, (void __user *)arg, sizeof(val));
+ if (rv) {
+ rv = -EFAULT;
+ goto exit;
+ }
+ wValue = val ? 1 : 0;
+ } else {
+ wValue = 0;
+ }
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ cmd,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ wValue,
+ data->ifnum,
+ buffer, 0x01, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+ goto exit;
+ } else if (rv != 1) {
+ dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "simple control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+ rv = 0;
+
+ exit:
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1185,6 +1241,21 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
+
+ case USBTMC488_IOCTL_REN_CONTROL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_REN_CONTROL);
+ break;
+
+ case USBTMC488_IOCTL_GOTO_LOCAL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_GOTO_LOCAL);
+ break;
+
+ case USBTMC488_IOCTL_LOCAL_LOCKOUT:
+ retval = usbtmc488_ioctl_simple(data, arg,
+
USBTMC488_REQUEST_LOCAL_LOCKOUT);
+ break;
}
skip_io_on_zombie:
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 1dc3af1..0d852c9 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -33,6 +33,9 @@
#define USBTMC_REQUEST_GET_CAPABILITIES7
#define USBTMC_REQUEST_INDICATOR_PULSE 64
#define USBTMC488_REQUEST_READ_STATUS_BYTE 128
+#define USBTMC488_REQUEST_REN_CONTROL 160
+#define USBTMC488_REQUEST_GOTO_LOCAL 161
+#define USBTMC488_REQUEST_LOCAL_LOCKOUT162
/* Request values for USBTMC driver's ioctl entry point */
#define USBTMC_IOC_NR 91
@@ -44,6 +47,9 @@
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+#define USBTMC488_IOCTL_REN_CONTROL_IOW(USBTMC_IOC_NR, 19, unsigned char)
+#define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20)
+#define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21)
/* Driver encoded usb488 capabilities */
#define USBTMC488_CAPABILITY_TRIGGER 1
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
Background:
When performing a read on an instrument that is executing a function
that runs longer than the USB timeout the instrument may hang and
require a device reset to recover. The READ_STATUS_BYTE operation
always returns even when the instrument is busy permitting to poll
for the appropriate condition. This capability is referred to in
instrument application notes on synchronizing acquisitions for other
platforms.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 219 +++
include/uapi/linux/usb/tmc.h | 2 +
2 files changed, 221 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..72ef7f0 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
+ /* data for interrupt in endpoint handling */
+ u8 bNotify1;
+ u8 bNotify2;
+ u16ifnum;
+ u8 iin_bTag;
+ u8*iin_buffer;
+ atomic_t iin_data_valid;
+ unsigned int iin_ep;
+ intiin_ep_present;
+ intiin_interval;
+ struct urb*iin_urb;
+ u16iin_wMaxPacketSize;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -99,6 +112,7 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities capabilities;
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
+ wait_queue_head_t waitq;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -373,6 +387,88 @@ exit:
return rv;
}
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+ unsigned long arg)
+{
+ u8 *buffer;
+ struct device *dev;
+ int rv;
+ u8 tag, stb;
+
+ dev = >intf->dev;
+
+ dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+ data->iin_ep_present);
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ atomic_set(>iin_data_valid, 0);
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC488_REQUEST_READ_STATUS_BYTE,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ data->iin_bTag,
+ data->ifnum,
+ buffer, 0x03, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (data->iin_ep_present) {
+
+ rv = wait_event_interruptible_timeout(
+ data->waitq,
+ atomic_read(>iin_data_valid) != 0,
+ USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_dbg(dev, "wait interrupted %d\n", rv);
+ goto exit;
+ }
+
+ if (rv == 0) {
+ dev_dbg(dev, "wait timed out\n");
+ rv = -ETIME;
+ goto exit;
+ }
+
+ tag = data->bNotify1 & 0x7f;
+
+ if (tag != data->iin_bTag) {
+ dev_err(dev, "expected bTag %x got %x\n",
+ data->iin_bTag, tag);
+ }
+
+ stb = data->bNotify2;
+ } else {
+ stb = buffer[2];
+ }
+
+ rv = copy_to_user((void __user *)arg, , sizeof(stb));
+ if (rv)
+ rv = -EFAULT;
+
+ exit:
+ /* bump interrupt bTag */
+ data->iin_bTag += 1;
+ if (data->iin_bTag > 127)
+ data->iin_bTag = 2;
+
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1069,6 +1165,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC_IOCTL_ABORT_BULK_IN:
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+
+ case USBTMC488_IOCTL_READ_STB:
+ retval = usbtmc488_ioctl_read_stb(data, arg);
+ break;
+
}
skip_io_on_zombie:
@@ -1092,6 +1193,69 @@ static struct usb_class_driver usbtmc_class = {
.minor_base = USBTMC_MI
[PATCH v4 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec
Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) and SRQ notifications with fasync (2/5) and poll/select (3/5) in order to be able to synchronize with variable duration instrument operations. Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and LOCAL_LOCKOUT. (4/5) Add convenience ioctl to return all device capabilities (5/5) PATCH Changelog: v4 - Remove excess newlines and parens - Simplify some expressions - Remove redundant initialisation v3 - Split into multiple patches as per gregkh request V2 - Fix V1 bug: not waking sleepers on disconnect. - Correct sparse warnings. V1 - Original patch Testing: All functions tested ok on an USBTMC-USB488 compliant oscilloscope Dave Penkler (5): Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Add support for USBTMC USB488 SRQ notification with fasync Add support for receiving USBTMC USB488 SRQ notifications via poll/select Add ioctl to retrieve USBTMC-USB488 capabilities Add ioctls to enable and disable local controls on an instrument drivers/usb/class/usbtmc.c | 354 +++ include/uapi/linux/usb/tmc.h | 29 +++- 2 files changed, 380 insertions(+), 3 deletions(-) -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 5/5] Add ioctls to enable and disable local controls on an instrument
These ioctls provide support for the USBTMC-USB488 control requests
for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 76
include/uapi/linux/usb/tmc.h | 6
2 files changed, 82 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index add0ce2..79caa78 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -478,6 +478,68 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
return rv;
}
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+ unsigned long arg,
+ unsigned int cmd)
+{
+ u8 *buffer;
+ struct device *dev;
+ int rv;
+ unsigned int val;
+ u16 wValue;
+
+ dev = >intf->dev;
+
+ if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+ return -EINVAL;
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+ rv = copy_from_user(, (void __user *)arg, sizeof(val));
+ if (rv) {
+ rv = -EFAULT;
+ goto exit;
+ }
+ wValue = (val) ? 1 : 0;
+ } else {
+ wValue = 0;
+ }
+
+ dev_warn(dev, "wValue is %d\n", wValue);
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ cmd,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ wValue,
+ data->ifnum,
+ buffer, 0x01, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+ goto exit;
+ } else if (rv != 1) {
+ dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "simple control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ } else {
+ rv = 0;
+ }
+
+ exit:
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1188,6 +1250,20 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
+ case USBTMC488_IOCTL_REN_CONTROL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_REN_CONTROL);
+ break;
+
+ case USBTMC488_IOCTL_GOTO_LOCAL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_GOTO_LOCAL);
+ break;
+
+ case USBTMC488_IOCTL_LOCAL_LOCKOUT:
+ retval = usbtmc488_ioctl_simple(data, arg,
+
USBTMC488_REQUEST_LOCAL_LOCKOUT);
+ break;
}
skip_io_on_zombie:
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 1dc3af1..0d852c9 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -33,6 +33,9 @@
#define USBTMC_REQUEST_GET_CAPABILITIES7
#define USBTMC_REQUEST_INDICATOR_PULSE 64
#define USBTMC488_REQUEST_READ_STATUS_BYTE 128
+#define USBTMC488_REQUEST_REN_CONTROL 160
+#define USBTMC488_REQUEST_GOTO_LOCAL 161
+#define USBTMC488_REQUEST_LOCAL_LOCKOUT162
/* Request values for USBTMC driver's ioctl entry point */
#define USBTMC_IOC_NR 91
@@ -44,6 +47,9 @@
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+#define USBTMC488_IOCTL_REN_CONTROL_IOW(USBTMC_IOC_NR, 19, unsigned char)
+#define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20)
+#define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21)
/* Driver encoded usb488 capabilities */
#define USBTMC488_CAPABILITY_TRIGGER 1
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/5] Add support for USBTMC USB488 SRQ notification with fasync
Background:
By configuring an instrument's event status register various
conditions can be reported via an SRQ notification. This complements
the synchronous polling approach using the READ_STATUS_BYTE ioctl
with an asynchronous notification.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 72ef7f0..7430a52 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -99,6 +99,7 @@ struct usbtmc_device_data {
intiin_interval;
struct urb*iin_urb;
u16iin_wMaxPacketSize;
+ atomic_t srq_asserted;
u8 rigol_quirk;
@@ -113,6 +114,7 @@ struct usbtmc_device_data {
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
wait_queue_head_t waitq;
+ struct fasync_struct *fasync;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -406,6 +408,9 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
atomic_set(>iin_data_valid, 0);
+ /* must issue read_stb before using poll or select */
+ atomic_set(>srq_asserted, 0);
+
rv = usb_control_msg(data->usb_dev,
usb_rcvctrlpipe(data->usb_dev, 0),
USBTMC488_REQUEST_READ_STATUS_BYTE,
@@ -1177,6 +1182,13 @@ skip_io_on_zombie:
return retval;
}
+static int usbtmc_fasync(int fd, struct file *file, int on)
+{
+ struct usbtmc_device_data *data = file->private_data;
+
+ return fasync_helper(fd, file, on, >fasync);
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1184,6 +1196,7 @@ static const struct file_operations fops = {
.open = usbtmc_open,
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
+ .fasync = usbtmc_fasync,
.llseek = default_llseek,
};
@@ -1214,6 +1227,16 @@ static void usbtmc_interrupt(struct urb *urb)
wake_up_interruptible(>waitq);
goto exit;
}
+ /* check for SRQ notification */
+ if ((data->iin_buffer[0] & 0x7f) == 1) {
+ if (data->fasync)
+ kill_fasync(>fasync,
+ SIGIO, POLL_IN);
+
+ atomic_set(>srq_asserted, 1);
+ wake_up_interruptible(>waitq);
+ goto exit;
+ }
}
dev_warn(>intf->dev, "invalid notification: %x\n",
data->iin_buffer[0]);
@@ -1280,6 +1303,7 @@ static int usbtmc_probe(struct usb_interface *intf,
mutex_init(>io_mutex);
init_waitqueue_head(>waitq);
atomic_set(>iin_data_valid, 0);
+ atomic_set(>srq_asserted, 0);
data->zombie = 0;
/* Determine if it is a Rigol or not */
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/5] Add support for receiving USBTMC USB488 SRQ notifications via poll/select
Background:
In many situations operations on multiple instruments need to be
synchronized. poll/select provide a convenient way of waiting on a
number of different instruments and other peripherals
simultaneously.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7430a52..6c0e1dc 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1189,6 +1190,27 @@ static int usbtmc_fasync(int fd, struct file *file, int
on)
return fasync_helper(fd, file, on, >fasync);
}
+static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
+{
+ struct usbtmc_device_data *data = file->private_data;
+ unsigned int mask;
+
+ mutex_lock(>io_mutex);
+
+ if (data->zombie) {
+ mask = POLLHUP | POLLERR;
+ goto no_poll;
+ }
+
+ poll_wait(file, >waitq, wait);
+
+ mask = (atomic_read(>srq_asserted)) ? POLLIN | POLLRDNORM : 0;
+
+no_poll:
+ mutex_unlock(>io_mutex);
+ return mask;
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1197,6 +1219,7 @@ static const struct file_operations fops = {
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
.fasync = usbtmc_fasync,
+ .poll = usbtmc_poll,
.llseek = default_llseek,
};
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 4/5] Add ioctl to retrieve USBTMC-USB488 capabilities
This is a convenience function to obtain an instrument's
capabilities from its file descriptor without having to access sysfs
from the user program.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 12
include/uapi/linux/usb/tmc.h | 21 ++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6c0e1dc..add0ce2 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -102,6 +102,9 @@ struct usbtmc_device_data {
u16iin_wMaxPacketSize;
atomic_t srq_asserted;
+ /* coalesced usb488_caps from usbtmc_dev_capabilities */
+ u8 usb488_caps;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -997,6 +1000,7 @@ static int get_capabilities(struct usbtmc_device_data
*data)
data->capabilities.device_capabilities = buffer[5];
data->capabilities.usb488_interface_capabilities = buffer[14];
data->capabilities.usb488_device_capabilities = buffer[15];
+ data->usb488_caps = (buffer[14] & 0x07) | ((buffer[15] & 0x0f) << 4);
rv = 0;
err_out:
@@ -1172,6 +1176,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+ case USBTMC488_IOCTL_GET_CAPS:
+ retval = copy_to_user((void __user *)arg,
+ >usb488_caps,
+ sizeof(data->usb488_caps));
+ if (retval)
+ retval = -EFAULT;
+ break;
+
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 7e5ced8..1dc3af1 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -2,12 +2,14 @@
* Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
* Copyright (C) 2008 Novell, Inc.
* Copyright (C) 2008 Greg Kroah-Hartman <gre...@suse.de>
+ * Copyright (C) 2015 Dave Penkler <dpenk...@gmail.com>
*
* This file holds USB constants defined by the USB Device Class
- * Definition for Test and Measurement devices published by the USB-IF.
+ * and USB488 Subclass Definitions for Test and Measurement devices
+ * published by the USB-IF.
*
- * It also has the ioctl definitions for the usbtmc kernel driver that
- * userspace needs to know about.
+ * It also has the ioctl and capability definitions for the
+ * usbtmc kernel driver that userspace needs to know about.
*/
#ifndef __LINUX_USB_TMC_H
@@ -40,6 +42,19 @@
#define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4)
#define USBTMC_IOCTL_CLEAR_OUT_HALT_IO(USBTMC_IOC_NR, 6)
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
+#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+/* Driver encoded usb488 capabilities */
+#define USBTMC488_CAPABILITY_TRIGGER 1
+#define USBTMC488_CAPABILITY_SIMPLE 2
+#define USBTMC488_CAPABILITY_REN_CONTROL 2
+#define USBTMC488_CAPABILITY_GOTO_LOCAL 2
+#define USBTMC488_CAPABILITY_LOCAL_LOCKOUT 2
+#define USBTMC488_CAPABILITY_488_DOT_2 4
+#define USBTMC488_CAPABILITY_DT1 16
+#define USBTMC488_CAPABILITY_RL1 32
+#define USBTMC488_CAPABILITY_SR1 64
+#define USBTMC488_CAPABILITY_FULL_SCPI 128
+
#endif
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] Add ioctls to enable and disable local controls on an instrument
Hi Andy, On Wed, Nov 11, 2015 at 09:36:41PM +0200, Andy Shevchenko wrote: > On Wed, Nov 11, 2015 at 1:21 PM, Dave Penkler <dpenk...@gmail.com> wrote: > > These ioctls provide support for the USBTMC-USB488 control requests > > for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT snip > > + goto exit; > > + } > > + wValue = (val) ? 1 : 0; > > !!val; I checked by compiling both variants - the compiler emits identical code for both so I prefer to keep the original as it is more obvious for maintainers reading the code. regards, -Dave -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/12] usb: early: add support for early printk through USB3 debug port
On 11/13/15 at 10:04am, Lu, Baolu wrote: > Hi Dave, > > On 11/12/2015 02:01 PM, Dave Young wrote: > >Hi, Baolu > > > >On 11/12/15 at 10:45am, Lu, Baolu wrote: > >>>Hi Dave, > >>> > >>>Which device are you testing with? This implementation was developed > >>>and tested on Intel Skylake devices. > >>> > >>>It doesn't surprise me if it doesn't work with other silicons. But it do > >>>remind me to create a verified-list and put those known-to-work devices > >>>in it. > >>> > >I have not got time to do more test. What infomation do you want? > >lsusb -v? > > lspci -v > Here is the controller information: Host side: 00:14.0 USB controller: Intel Corporation 8 Series/C220 Series Chipset Family USB xHCI (rev 05) (prog-if 30 [XHCI]) Subsystem: Hewlett-Packard Company Device 1905 Flags: bus master, medium devsel, latency 0, IRQ 27 Memory at ef12 (64-bit, non-prefetchable) [size=64K] Capabilities: Kernel driver in use: xhci_hcd Target side: 00:14.0 USB controller: Intel Corporation 8 Series USB xHCI HC (rev 04) (prog-if 30 [XHCI]) Subsystem: Lenovo Device 220c Flags: bus master, medium devsel, latency 0, IRQ 41 Memory at f062 (64-bit, non-prefetchable) [size=64K] Capabilities: Kernel driver in use: xhci_hcd Thanks Dave -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
Background:
When performing a read on an instrument that is executing a function
that runs longer than the USB timeout the instrument may hang and require
a device reset to recover. The READ_STATUS_BYTE operation always returns
even when the instrument is busy permitting to poll for the appropriate
condition. This capability is refered to in instrument application notes
on synchronizing acquisitions for other platforms.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 223 +++
include/uapi/linux/usb/tmc.h | 2 +
2 files changed, 225 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..c1593e7 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -87,6 +87,20 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
+
+ /* data for interrupt in endpoint handling */
+ u8 bNotify1;
+ u8 bNotify2;
+ u16ifnum;
+ u8 iin_bTag;
+ u8*iin_buffer;
+ atomic_t iin_data_valid;
+ unsigned int iin_ep;
+ intiin_ep_present;
+ intiin_interval;
+ struct urb*iin_urb;
+ u16iin_wMaxPacketSize;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -99,6 +113,7 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities capabilities;
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
+ wait_queue_head_t waitq;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -373,6 +388,91 @@ exit:
return rv;
}
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+ unsigned long arg)
+{
+ u8 *buffer;
+ struct device *dev;
+ int rv;
+ u8 tag, stb;
+
+ dev = >intf->dev;
+
+ dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+ data->iin_ep_present);
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+
+ atomic_set(>iin_data_valid, 0);
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC488_REQUEST_READ_STATUS_BYTE,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ data->iin_bTag,
+ data->ifnum,
+ buffer, 0x03, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "control status returned %x\n",
+ buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (data->iin_ep_present) {
+
+ rv = wait_event_interruptible_timeout(
+ data->waitq,
+ (atomic_read(>iin_data_valid) != 0),
+ USBTMC_TIMEOUT
+ );
+
+ if (rv < 0) {
+ dev_dbg(dev, "wait interrupted %d\n", rv);
+ goto exit;
+ }
+
+ if (rv == 0) {
+ dev_dbg(dev, "wait timed out\n");
+ rv = -ETIME;
+ goto exit;
+ }
+
+ tag = data->bNotify1 & 0x7f;
+
+ if (tag != data->iin_bTag) {
+ dev_err(dev, "expected bTag %x got %x\n",
+ data->iin_bTag, tag);
+ }
+
+ stb = data->bNotify2;
+ } else {
+ stb = buffer[2];
+ }
+
+ rv = copy_to_user((void __user *)arg, , sizeof(stb));
+ if (rv)
+ rv = -EFAULT;
+
+ exit:
+ /* bump interrupt bTag */
+ data->iin_bTag += 1;
+ if (data->iin_bTag > 127)
+ data->iin_bTag = 2;
+
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1069,6 +1169,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC_IOCTL_ABORT_BULK_IN:
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+
+ case USBTMC488_IOCTL_READ_STB:
+ retval = usbtmc488_ioctl_read_stb(data, arg);
+ break;
+
}
skip_io_on_zombie:
@@ -1092,6 +1197,69 @@ static struct usb_class_driv
[PATCH v3 2/5] Add support for USBTMC USB488 SRQ notification with fasync
Background:
By configuring an instrument's event status register various conditions
can be reported via an SRQ notification. This complements the synchronous
polling approach using the READ_STATUS_BYTE ioctl with an asynchronous
notification.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index c1593e7..2239cd0 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -100,6 +100,7 @@ struct usbtmc_device_data {
intiin_interval;
struct urb*iin_urb;
u16iin_wMaxPacketSize;
+ atomic_t srq_asserted;
u8 rigol_quirk;
@@ -114,6 +115,7 @@ struct usbtmc_device_data {
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
wait_queue_head_t waitq;
+ struct fasync_struct *fasync;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -408,6 +410,9 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
atomic_set(>iin_data_valid, 0);
+ /* must issue read_stb before using poll or select */
+ atomic_set(>srq_asserted, 0);
+
rv = usb_control_msg(data->usb_dev,
usb_rcvctrlpipe(data->usb_dev, 0),
USBTMC488_REQUEST_READ_STATUS_BYTE,
@@ -1181,6 +1186,13 @@ skip_io_on_zombie:
return retval;
}
+static int usbtmc_fasync(int fd, struct file *file, int on)
+{
+ struct usbtmc_device_data *data = file->private_data;
+
+ return fasync_helper(fd, file, on, >fasync);
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1188,6 +1200,7 @@ static const struct file_operations fops = {
.open = usbtmc_open,
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
+ .fasync = usbtmc_fasync,
.llseek = default_llseek,
};
@@ -1218,6 +1231,16 @@ static void usbtmc_interrupt(struct urb *urb)
wake_up_interruptible(>waitq);
goto exit;
}
+ /* check for SRQ notification */
+ if ((data->iin_buffer[0] & 0x7f) == 1) {
+ if (data->fasync)
+ kill_fasync(>fasync,
+ SIGIO, POLL_IN);
+
+ atomic_set(>srq_asserted, 1);
+ wake_up_interruptible(>waitq);
+ goto exit;
+ }
}
dev_warn(>intf->dev, "invalid notification: %x\n",
data->iin_buffer[0]);
@@ -1284,6 +1307,7 @@ static int usbtmc_probe(struct usb_interface *intf,
mutex_init(>io_mutex);
init_waitqueue_head(>waitq);
atomic_set(>iin_data_valid, 0);
+ atomic_set(>srq_asserted, 0);
data->zombie = 0;
/* Determine if it is a Rigol or not */
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/5] Add support for receiving USBTMC USB488 SRQ notifications via poll/select
Background:
In many situations operations on multiple instruments need to be
synchronized. poll/select provide a convenient way of waiting on a number
of different instruments and other peripherals simultaneously.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 2239cd0..bb9a6ab 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1193,6 +1194,27 @@ static int usbtmc_fasync(int fd, struct file *file, int
on)
return fasync_helper(fd, file, on, >fasync);
}
+static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
+{
+ struct usbtmc_device_data *data = file->private_data;
+ unsigned int mask = 0;
+
+ mutex_lock(>io_mutex);
+
+ if (data->zombie) {
+ mask = POLLHUP | POLLERR;
+ goto no_poll;
+ }
+
+ poll_wait(file, >waitq, wait);
+
+ mask = (atomic_read(>srq_asserted)) ? POLLIN | POLLRDNORM : 0;
+
+no_poll:
+ mutex_unlock(>io_mutex);
+ return mask;
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1201,6 +1223,7 @@ static const struct file_operations fops = {
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
.fasync = usbtmc_fasync,
+ .poll = usbtmc_poll,
.llseek = default_llseek,
};
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec
Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) and SRQ notifications with fasync (2/5) and poll/select (3/5) in order to be able to synchronize with variable duration instrument operations. Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and LOCAL_LOCKOUT. (4/5) Add convenience ioctl to return all device capabilities (5/5) PATCH Changelog: v3 - Split into multiple patches as per gregkh request V2 - Fix V1 bug: not waking sleepers on disconnect. - Correct sparse warnings. V1 - Original patch Testing: All functions tested ok on an USBTMC-USB488 compliant oscilloscope Dave Penkler (5): Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Add support for USBTMC USB488 SRQ notification with fasync Add support for receiving USBTMC USB488 SRQ notifications via poll/select Add ioctl to retrieve USBTMC-USB488 capabilities Add ioctls to enable and disable local controls on an instrument drivers/usb/class/usbtmc.c | 358 +++ include/uapi/linux/usb/tmc.h | 30 +++- 2 files changed, 385 insertions(+), 3 deletions(-) -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/5] Add ioctls to enable and disable local controls on an instrument
These ioctls provide support for the USBTMC-USB488 control requests
for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 76
include/uapi/linux/usb/tmc.h | 9 +-
2 files changed, 84 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index deca4b5..646bfff 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -482,6 +482,68 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
return rv;
}
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+ unsigned long arg,
+ unsigned int cmd)
+{
+ u8 *buffer;
+ struct device *dev;
+ int rv;
+ unsigned int val;
+ u16 wValue;
+
+ dev = >intf->dev;
+
+ if (0 == (data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+ return -EINVAL;
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+
+ if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+ rv = copy_from_user(, (void __user *)arg, sizeof(val));
+ if (rv) {
+ rv = -EFAULT;
+ goto exit;
+ }
+ wValue = (val) ? 1 : 0;
+ } else {
+ wValue = 0;
+ }
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ cmd,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ wValue,
+ data->ifnum,
+ buffer, 0x01, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+ goto exit;
+ } else if (rv != 1) {
+ dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "simple control status returned %x\n",
+ buffer[0]);
+ rv = -EIO;
+ goto exit;
+ } else {
+ rv = 0;
+ }
+
+ exit:
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1192,6 +1254,20 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
+ case USBTMC488_IOCTL_REN_CONTROL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_REN_CONTROL);
+ break;
+
+ case USBTMC488_IOCTL_GOTO_LOCAL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_GOTO_LOCAL);
+ break;
+
+ case USBTMC488_IOCTL_LOCAL_LOCKOUT:
+ retval = usbtmc488_ioctl_simple(data, arg,
+
USBTMC488_REQUEST_LOCAL_LOCKOUT);
+ break;
}
skip_io_on_zombie:
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 2606664..fe9663f 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -1,3 +1,4 @@
+
/*
* Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
* Copyright (C) 2008 Novell, Inc.
@@ -32,7 +33,10 @@
#define USBTMC_REQUEST_CHECK_CLEAR_STATUS 6
#define USBTMC_REQUEST_GET_CAPABILITIES7
#define USBTMC_REQUEST_INDICATOR_PULSE 64
-#define USBTMC488_REQUEST_READ_STATUS_BYTE 128
+#define USBTMC488_REQUEST_READ_STATUS_BYTE 128
+#define USBTMC488_REQUEST_REN_CONTROL 160
+#define USBTMC488_REQUEST_GOTO_LOCAL 161
+#define USBTMC488_REQUEST_LOCAL_LOCKOUT162
/* Request values for USBTMC driver's ioctl entry point */
#define USBTMC_IOC_NR 91
@@ -44,6 +48,9 @@
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+#define USBTMC488_IOCTL_REN_CONTROL_IOW(USBTMC_IOC_NR, 19, unsigned char)
+#define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20)
+#define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21)
/* Driver encoded usb488 capabilities */
#define USBTMC488_CAPABILITY_TRIGGER 1
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to m
[PATCH v3 4/5] Add ioctl to retrieve USBTMC-USB488 capabilities
This is a convenience function to obtain an instrument's capabilities
from its file descriptor without having to access sysfs from the user
program.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 12
include/uapi/linux/usb/tmc.h | 21 ++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index bb9a6ab..deca4b5 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -103,6 +103,9 @@ struct usbtmc_device_data {
u16iin_wMaxPacketSize;
atomic_t srq_asserted;
+ /* coalesced usb488_caps from usbtmc_dev_capabilities */
+ u8 usb488_caps;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -1001,6 +1004,7 @@ static int get_capabilities(struct usbtmc_device_data
*data)
data->capabilities.device_capabilities = buffer[5];
data->capabilities.usb488_interface_capabilities = buffer[14];
data->capabilities.usb488_device_capabilities = buffer[15];
+ data->usb488_caps = (buffer[14] & 0x07) | ((buffer[15] & 0x0f) << 4);
rv = 0;
err_out:
@@ -1176,6 +1180,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+ case USBTMC488_IOCTL_GET_CAPS:
+ retval = copy_to_user((void __user *)arg,
+ >usb488_caps,
+ sizeof(data->usb488_caps));
+ if (retval)
+ retval = -EFAULT;
+ break;
+
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 49060ea..2606664 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -2,12 +2,14 @@
* Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
* Copyright (C) 2008 Novell, Inc.
* Copyright (C) 2008 Greg Kroah-Hartman <gre...@suse.de>
+ * Copyright (C) 2015 Dave Penkler <dpenk...@gmail.com>
*
* This file holds USB constants defined by the USB Device Class
- * Definition for Test and Measurement devices published by the USB-IF.
+ * and USB488 Subclass Definitions for Test and Measurement devices
+ * published by the USB-IF.
*
- * It also has the ioctl definitions for the usbtmc kernel driver that
- * userspace needs to know about.
+ * It also has the ioctl and capability definitions for the
+ * usbtmc kernel driver that userspace needs to know about.
*/
#ifndef __LINUX_USB_TMC_H
@@ -40,6 +42,19 @@
#define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4)
#define USBTMC_IOCTL_CLEAR_OUT_HALT_IO(USBTMC_IOC_NR, 6)
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
+#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+/* Driver encoded usb488 capabilities */
+#define USBTMC488_CAPABILITY_TRIGGER 1
+#define USBTMC488_CAPABILITY_SIMPLE 2
+#define USBTMC488_CAPABILITY_REN_CONTROL 2
+#define USBTMC488_CAPABILITY_GOTO_LOCAL 2
+#define USBTMC488_CAPABILITY_LOCAL_LOCKOUT 2
+#define USBTMC488_CAPABILITY_488_DOT_2 4
+#define USBTMC488_CAPABILITY_DT1 16
+#define USBTMC488_CAPABILITY_RL1 32
+#define USBTMC488_CAPABILITY_SR1 64
+#define USBTMC488_CAPABILITY_FULL_SCPI 128
+
#endif
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/12] usb: early: add support for early printk through USB3 debug port
Hi, Baolu On 11/12/15 at 10:45am, Lu, Baolu wrote: > Hi Dave, > > Which device are you testing with? This implementation was developed > and tested on Intel Skylake devices. > > It doesn't surprise me if it doesn't work with other silicons. But it do > remind me to create a verified-list and put those known-to-work devices > in it. > I have not got time to do more test. What infomation do you want? lsusb -v? > Thanks, > Baolu > > On 11/11/2015 10:25 AM, Dave Young wrote: > >Hi, > > > >On 11/11/15 at 09:32am, Lu, Baolu wrote: > >> > >>On 11/10/2015 05:39 PM, Dave Young wrote: > >>>Hi, > >>> > >>>On 11/09/15 at 03:38pm, Lu Baolu wrote: > >>>>This patch series adds support for early printk through USB3 debug port. > >>>>USB3 debug port is described in xHCI specification as an optional extended > >>>>capability. > >>>> > >>>I did a test with your previous patchset with the manually wired cable. > >>>debug host detected the remote device, but later the devie automaticlly > >>>disconnected and earlyprintk hangs. > >>Hi Dave, > >> > >>What I have done is: > >Retested it, seems it is not stable. I got a sucessful boot with earlyprintk > >But only once and there was no "Press Y to continue", I just blindly pressed > >Y. > > > >The other tests failed. > > > >Since it is not convinience to test, do you have way to enable the dbc > >after kernel boot? like echo 1 to a sysfs file to enable it. > >>(1) Build a new kernel for debug target with this patch series applied. > >>(2) Add "earlyprintk=xdbc" to the kernel option of debug target. The > >> "keep" option for early printk doesn't support yet. (That's my next > >> target.) > >> > >>(3) Boot the debug host, and disable USB runtime suspend: > >> > >># echo on > /sys/bus/pci/devices//power/control > >># echo on | tee /sys/bus/usb/devices/*/power/control > >> > >>(4) Boot the debug target. Check the dmesg message on debug host. > >> > >># tail -f /var/log/kern.log > >> > >>Nov 12 01:27:50 allen-ult kernel: [ 1815.983374] usb 4-3: new SuperSpeed USB > >>device number 4 using xhci_hcd > >>Nov 12 01:27:50 allen-ult kernel: [ 1815.999595] usb 4-3: LPM exit latency > >>is zeroed, disabling LPM. > >>Nov 12 01:27:50 allen-ult kernel: [ 1815.999899] usb 4-3: New USB device > >>found, idVendor=1d6b, idProduct=0004 > >>Nov 12 01:27:50 allen-ult kernel: [ 1815.02] usb 4-3: New USB device > >>strings: Mfr=1, Product=2, SerialNumber=3 > >>Nov 12 01:27:50 allen-ult kernel: [ 1815.03] usb 4-3: Product: Remote > >>GDB > >>Nov 12 01:27:50 allen-ult kernel: [ 1815.04] usb 4-3: Manufacturer: > >>Linux > >>Nov 12 01:27:50 allen-ult kernel: [ 1815.05] usb 4-3: SerialNumber: 0001 > >>Nov 12 01:27:50 allen-ult kernel: [ 1816.000240] usb_debug 4-3:1.0: xhci_dbc > >>converter detected > >>Nov 12 01:27:50 allen-ult kernel: [ 1816.000360] usb 4-3: xhci_dbc converter > >>now attached to ttyUSB0 > >> > >>(5) Host has completed enumeration of debug device. Start "minicom" on debug > >>host. > >> > >Most times I have no chance to run minicom before the usb disconnection here. > > > >>Welcome to minicom 2.7 > >> > >>OPTIONS: I18n > >>Compiled on Jan 1 2014, 17:13:19. > >>Port /dev/ttyUSB0, 01:28:02 > >> > >>Press CTRL-A Z for help on special keys > >> > >>Press Y to continue... > >> > >>(6) You should be able to see "Press Y to continue..." (if not, try pressing > >>Enter key) > >>Press Y key, debug target should go ahead with boot and early boot messages > >>should show in mincom. > >> > >>Press Y to continue... > >>[0.00] Initializing cgroup subsys cpuset > >>[0.00] Initializing cgroup subsys cpu > >>[0.00] Initializing cgroup subsys cpuacct > >>[0.00] Linux version 4.3.0-rc7+ (allen@blu-skl) (gcc version 4.8.4 > >>(Ubuntu 4.8.4-2ubuntu1~14.04) 5 > >>[0.00] Command line: BOOT_IMAGE=/boot/vmlinuz-4.3.0-rc7+ > >>root=UUID=5a2fb856-0238-4b6e-aa45-beeccb7 > >>[0.00] KERNEL supported cpus: > >> > >>[...skipped...] > >> > >>[0.00] Offload RCU callbacks from CPUs: 0-7. > >>[0.00] Console: colour du
Re: [PATCH v3 00/12] usb: early: add support for early printk through USB3 debug port
Hi, On 11/09/15 at 03:38pm, Lu Baolu wrote: > This patch series adds support for early printk through USB3 debug port. > USB3 debug port is described in xHCI specification as an optional extended > capability. > I did a test with your previous patchset with the manually wired cable. debug host detected the remote device, but later the devie automaticlly disconnected and earlyprintk hangs. I have not got more time, will try your new patchset when I have time. Thanks Dave -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/12] usb: xhci: add sysfs file for xHCI debug port
[snip]
> diff --git a/drivers/usb/host/xhci-sysfs.c b/drivers/usb/host/xhci-sysfs.c
> new file mode 100644
> index 000..0192ac4
> --- /dev/null
> +++ b/drivers/usb/host/xhci-sysfs.c
> @@ -0,0 +1,100 @@
> +/*
> + * sysfs interface for xHCI host controller driver
> + *
> + * Copyright (C) 2015 Intel Corp.
> + *
> + * Author: Lu Baolu <baolu...@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include
> +
> +#include "xhci.h"
> +
> +/*
> + * Return the register offset of a extended capability specified
> + * by @cap_id. Return 0 if @cap_id capability is not supported or
> + * in error cases.
> + */
> +static int get_extended_capability_offset(struct xhci_hcd *xhci,
> + int cap_id)
> +{
> + u32 cap_reg;
> + unsigned long flags;
> + int offset;
> + void __iomem*base = (void __iomem *) xhci->cap_regs;
> + struct usb_hcd *hcd = xhci_to_hcd(xhci);
> + int time_to_leave = XHCI_EXT_MAX_CAPID;
> +
> + spin_lock_irqsave(>lock, flags);
> +
> + offset = xhci_find_next_cap_offset(base, XHCI_HCC_PARAMS_OFFSET);
> + if (!HCD_HW_ACCESSIBLE(hcd) || !offset) {
> + spin_unlock_irqrestore(>lock, flags);
> + return 0;
> + }
> +
> + while (time_to_leave--) {
> + cap_reg = readl(base + offset);
> +
> + if (XHCI_EXT_CAPS_ID(cap_reg) == cap_id)
> + break;
> +
> + offset = xhci_find_next_cap_offset(base, offset);
> + if (!offset)
> + break;
> + }
> +
> + spin_unlock_irqrestore(>lock, flags);
I'm not sure spin_lock is good and necessary here, also seems there's already
a function to find the cap offset:
xhci_find_ext_cap_by_id() in xhci-ext-caps.h
> +
> + return offset;
> +}
> +
Thanks
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/12] usb: early: add support for early printk through USB3 debug port
Hi, On 11/11/15 at 09:32am, Lu, Baolu wrote: > > > On 11/10/2015 05:39 PM, Dave Young wrote: > >Hi, > > > >On 11/09/15 at 03:38pm, Lu Baolu wrote: > >>This patch series adds support for early printk through USB3 debug port. > >>USB3 debug port is described in xHCI specification as an optional extended > >>capability. > >> > >I did a test with your previous patchset with the manually wired cable. > >debug host detected the remote device, but later the devie automaticlly > >disconnected and earlyprintk hangs. > > Hi Dave, > > What I have done is: Retested it, seems it is not stable. I got a sucessful boot with earlyprintk But only once and there was no "Press Y to continue", I just blindly pressed Y. The other tests failed. Since it is not convinience to test, do you have way to enable the dbc after kernel boot? like echo 1 to a sysfs file to enable it. > > (1) Build a new kernel for debug target with this patch series applied. > (2) Add "earlyprintk=xdbc" to the kernel option of debug target. The > "keep" option for early printk doesn't support yet. (That's my next > target.) > > (3) Boot the debug host, and disable USB runtime suspend: > > # echo on > /sys/bus/pci/devices//power/control > # echo on | tee /sys/bus/usb/devices/*/power/control > > (4) Boot the debug target. Check the dmesg message on debug host. > > # tail -f /var/log/kern.log > > Nov 12 01:27:50 allen-ult kernel: [ 1815.983374] usb 4-3: new SuperSpeed USB > device number 4 using xhci_hcd > Nov 12 01:27:50 allen-ult kernel: [ 1815.999595] usb 4-3: LPM exit latency > is zeroed, disabling LPM. > Nov 12 01:27:50 allen-ult kernel: [ 1815.999899] usb 4-3: New USB device > found, idVendor=1d6b, idProduct=0004 > Nov 12 01:27:50 allen-ult kernel: [ 1815.02] usb 4-3: New USB device > strings: Mfr=1, Product=2, SerialNumber=3 > Nov 12 01:27:50 allen-ult kernel: [ 1815.03] usb 4-3: Product: Remote > GDB > Nov 12 01:27:50 allen-ult kernel: [ 1815.04] usb 4-3: Manufacturer: > Linux > Nov 12 01:27:50 allen-ult kernel: [ 1815.05] usb 4-3: SerialNumber: 0001 > Nov 12 01:27:50 allen-ult kernel: [ 1816.000240] usb_debug 4-3:1.0: xhci_dbc > converter detected > Nov 12 01:27:50 allen-ult kernel: [ 1816.000360] usb 4-3: xhci_dbc converter > now attached to ttyUSB0 > > (5) Host has completed enumeration of debug device. Start "minicom" on debug > host. > Most times I have no chance to run minicom before the usb disconnection here. > > Welcome to minicom 2.7 > > OPTIONS: I18n > Compiled on Jan 1 2014, 17:13:19. > Port /dev/ttyUSB0, 01:28:02 > > Press CTRL-A Z for help on special keys > > Press Y to continue... > > (6) You should be able to see "Press Y to continue..." (if not, try pressing > Enter key) > Press Y key, debug target should go ahead with boot and early boot messages > should show in mincom. > > Press Y to continue... > [0.00] Initializing cgroup subsys cpuset > [0.00] Initializing cgroup subsys cpu > [0.00] Initializing cgroup subsys cpuacct > [0.00] Linux version 4.3.0-rc7+ (allen@blu-skl) (gcc version 4.8.4 > (Ubuntu 4.8.4-2ubuntu1~14.04) 5 > [0.00] Command line: BOOT_IMAGE=/boot/vmlinuz-4.3.0-rc7+ > root=UUID=5a2fb856-0238-4b6e-aa45-beeccb7 > [0.00] KERNEL supported cpus: > > [...skipped...] > > [0.00] Offload RCU callbacks from CPUs: 0-7. > [0.00] Console: colour dummy device 80x25 > [0.00] console [tty0] enabled > [0.00] bootconsole [earlyxdbc0] disabled > > > So "the devie automaticlly disconnected and earlyprintk hangs" happens in > which step? > Here is some log on host side. [ 1568.052135] usb 2-2: new SuperSpeed USB device number 5 using xhci_hcd [ 1568.063416] usb 2-2: LPM exit latency is zeroed, disabling LPM. [ 1568.063750] usb 2-2: New USB device found, idVendor=1d6b, idProduct=0004 [ 1568.063751] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 1568.063752] usb 2-2: Product: Remote GDB [ 1568.063753] usb 2-2: Manufacturer: Linux [ 1568.063754] usb 2-2: SerialNumber: 0001 [ 1568.065580] usb_debug 2-2:1.0: xhci_dbc converter detected [ 1568.066309] usb 2-2: xhci_dbc converter now attached to ttyUSB0 [ 1580.464706] usb 2-2: USB disconnect, device number 5 [ 1580.464996] xhci_dbc ttyUSB0: xhci_dbc converter now disconnected from ttyUSB0 [ 1580.465062] usb_debug 2-2:1.0: device disconnected [ 1580.670743] usb 2-2: new SuperSpeed USB device number 6 using xhci_hcd [ 1580.682068] usb 2-2: LPM exit latency is zeroed, disabling LPM. [ 1580.682667] usb 2-2: New USB device found, idVendor=1d6b, idProduct=0004 [ 1580.68267
Re: [PATCH 00/12] usb: early: add support for early printk through USB3 debug port
On 10/28/15 at 04:00pm, Lu Baolu wrote: > This patch series adds support for early printk through USB3 debug port. > USB3 debug port is described in xHCI specification as an optional extended > capability. > > The first patch adds a file in debugfs, through which users can check > whether the debug capability is supported by a specific host controller. > > Patch 2 to 10 add the driver for xHCI debug capability. It interfaces with > the register set and provides the required ops (read/write/control) to upper > layers. Early printk is one consumer of these ops. The hooks for early printk > are introduced in patch 9. This design is similar to what we have done in > drivers/usb/early/ehci-dbgp.c. > > Patch 11 is a minor change to usb_debug module. This change is required to > bind usb_debug with the USB3 debug device. > > Patch 12 is the design document and user guide. > Nice work, I want to try your patches. But I have only one machine with debug capability, I think I can use it as debug target. Can I use another machine with usb 3.0 but no debug capability as the debug host? BTW, I hacked a cable according to usb 3 spec, cut vbus/d+/d-, cross wired ss pins. Thanks Dave -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] USB: usbtmc: Add support for missing USBTMC-USB488 spec
Implement support for the USB488 defined READ_STATUS_BYTE and SRQ
notifications with ioctl, fasync and poll/select in order to be able
to synchronize with variable duration instrument operations.
Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and
LOCAL_LOCKOUT.
Add convenience ioctl to return all device capabilities.
Fix PATCH V1 bug: not waking sleepers on disconnect.
Correct sparse warnings.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 359 +++
include/uapi/linux/usb/tmc.h | 42 +++--
2 files changed, 391 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..48e6efc 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -87,6 +88,24 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
+
+ /* data for interrupt in endpoint handling */
+ u8 bNotify1;
+ u8 bNotify2;
+ u16ifnum;
+ u8 iin_bTag;
+ u8*iin_buffer;
+ atomic_t iin_data_valid;
+ unsigned int iin_ep;
+ intiin_ep_present;
+ intiin_interval;
+ struct urb*iin_urb;
+ u16iin_wMaxPacketSize;
+ atomic_t srq_asserted;
+
+ /* coalesced usb488_caps from usbtmc_dev_capabilities */
+ u8 usb488_caps;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -99,6 +118,8 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities capabilities;
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
+ wait_queue_head_t waitq;
+ struct fasync_struct *fasync;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -373,6 +394,157 @@ exit:
return rv;
}
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+ unsigned long arg)
+{
+ u8 *buffer;
+ struct device *dev;
+ int rv;
+ u8 tag, stb;
+
+ dev = >intf->dev;
+
+ dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+ data->iin_ep_present);
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+
+ atomic_set(>iin_data_valid, 0);
+
+ /* must issue read_stb before using poll or select */
+ atomic_set(>srq_asserted, 0);
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC488_REQUEST_READ_STATUS_BYTE,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ data->iin_bTag,
+ data->ifnum,
+ buffer, 0x03, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "control status returned %x\n",
+ buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (data->iin_ep_present) {
+
+ rv = wait_event_interruptible_timeout(
+ data->waitq,
+ (atomic_read(>iin_data_valid) != 0),
+ USBTMC_TIMEOUT
+ );
+
+ if (rv < 0) {
+ dev_dbg(dev, "wait interrupted %d\n", rv);
+ goto exit;
+ }
+
+ if (rv == 0) {
+ dev_dbg(dev, "wait timed out\n");
+ rv = -ETIME;
+ goto exit;
+ }
+
+ tag = data->bNotify1 & 0x7f;
+
+ if (tag != data->iin_bTag) {
+ dev_err(dev, "expected bTag %x got %x\n",
+ data->iin_bTag, tag);
+ }
+
+ stb = data->bNotify2;
+ } else {
+ stb = buffer[2];
+ }
+
+ rv = copy_to_user((void __user *)arg, , sizeof(stb));
+ if (rv)
+ rv = -EFAULT;
+
+ exit:
+ /* bump interrupt bTag */
+ data->iin_bTag += 1;
+ if (data->iin_bTag > 127)
+ data->iin_bTag = 2;
+
+ kfree(buffer);
+ return rv;
+}
+
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+ unsigned long arg,
+ unsigned int cmd)
+{
+ u8 *buffe
Re: [PATCH] USB: usbtmc: Add support for missing USBTMC-USB488 spec
Hi Oliver,
On Wed, Oct 14, 2015 at 3:33 PM, Oliver Neukum <oneu...@suse.com> wrote:
> On Wed, 2015-10-14 at 15:13 +0200, dave penkler wrote:
>
> Hi,
>
> just a few remarks.
thank you.
>
>>
>> +static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
>> + unsigned long arg)
>> +{
>> + u8 *buffer;
>> + struct device *dev;
>> + int rv;
>> + u8 tag, stb;
>> +
>> + dev = >intf->dev;
>> +
>> + dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
>> + data->iin_ep_present);
>> +
>> + buffer = kmalloc(8, GFP_KERNEL);
>> + if (!buffer)
>> + return -ENOMEM;
>> +
>> +
>> + atomic_set(>iin_data_valid, 0);
>> +
>> + /* must issue read_stb before using poll or select */
>> + atomic_set(>srq_asserted, 0);
>> +
>> + rv = usb_control_msg(data->usb_dev,
>> + usb_rcvctrlpipe(data->usb_dev, 0),
>> + USBTMC488_REQUEST_READ_STATUS_BYTE,
>> + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> + data->iin_bTag,
>> + data->ifnum,
>> + buffer, 0x03, USBTMC_TIMEOUT);
>> +
>> + if (rv < 0) {
>> + dev_err(dev, "stb usb_control_msg returned %d\n", rv);
>> + goto exit;
>> + }
>> +
>> + if (buffer[0] != USBTMC_STATUS_SUCCESS) {
>> + dev_err(dev, "control status returned %x\n",
>> + buffer[0]);
>> + rv = -EIO;
>> + goto exit;
>> + }
>> +
>> + if (data->iin_ep_present) {
>> +
>> + rv = wait_event_interruptible_timeout(
>> + data->waitq,
>> + (atomic_read(>iin_data_valid) != 0),
>> + USBTMC_TIMEOUT
>> + );
>> +
>> + if (rv < 0) {
>> + dev_dbg(dev, "wait interrupted %d\n", rv);
>> + goto exit;
>> + }
>
> If you do this, yet the transfer succeeds, how do you keep the tag
> between host and device consistent?
>
The next message will just use the same tag value as before if rv <= 0
which is not a problem - one could do it either way. I'll move the bump
after exit: for V2
>> +
>> + if (rv == 0) {
>> + dev_dbg(dev, "wait timed out\n");
>> + rv = -ETIME;
>> + goto exit;
>> + }
>> +
>> + tag = data->bNotify1 & 0x7f;
>> +
>> + if (tag != data->iin_bTag) {
>> + dev_err(dev, "expected bTag %x got %x\n",
>> + data->iin_bTag, tag);
>> + }
>> +
>> + stb = data->bNotify2;
>> + } else {
>> + stb = buffer[2];
>> + }
>> +
>> + /* bump interrupt bTag */
>> + data->iin_bTag += 1;
>> + if (data->iin_bTag > 127)
>> + data->iin_bTag = 2;
>> +
>> + rv = copy_to_user((void *)arg, , sizeof(stb));
>> + if (rv)
>> + rv = -EFAULT;
>> +
>> + exit:
>> + kfree(buffer);
>> + return rv;
>> +
>> +}
>> +
>> +static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
>> +{
>> + struct usbtmc_device_data *data = file->private_data;
>> + unsigned int mask = 0;
>> +
>> + mutex_lock(>io_mutex);
>> +
>> + if (data->zombie)
>> + goto no_poll;
>> +
>> + poll_wait(file, >waitq, wait);
>
> Presumably the waiters should be woken when the device is disconnected.
>
Yes the mask should be set to POLLHUP etc on the first zombie test above.
After the poll_wait it should simply read
mask = (atomic_read(>srq_asserted)) ? POLLIN | POLLRDNORM : 0;
Will revise for V2
thank you
>> +
>> + mask = data->zombie ? POLLHUP | POLLERR :
>> + (atomic_read(>srq_asserted)) ? POLLIN | POLLRDNORM : 0;
>> +
>> +no_poll:
>> + mutex_unlock(>io_mutex);
>> + return mask;
>> +}
>
> Regards
> Oliver
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: usbtmc: Add support for missing USBTMC-USB488 spec
Implement support for the USB488 defined READ_STATUS_BYTE and SRQ
notifications with ioctl, fasync and poll/select in order to be able
to synchronize with variable duration instrument operations.
Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and
LOCAL_LOCKOUT.
Add convenience ioctl to return all device capabilities.
Signed-off-by: Dave Penkler <dpenk...@gmail.com>
---
drivers/usb/class/usbtmc.c | 359 +++
include/uapi/linux/usb/tmc.h | 42 +++--
2 files changed, 391 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..83c8416 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -87,6 +88,24 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
+
+ /* data for interrupt in endpoint handling */
+ u8 bNotify1;
+ u8 bNotify2;
+ u16ifnum;
+ u8 iin_bTag;
+ u8*iin_buffer;
+ atomic_t iin_data_valid;
+ unsigned int iin_ep;
+ intiin_ep_present;
+ intiin_interval;
+ struct urb*iin_urb;
+ u16iin_wMaxPacketSize;
+ atomic_t srq_asserted;
+
+ /* coalesced usb488_caps from usbtmc_dev_capabilities */
+ u8 usb488_caps;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -99,6 +118,8 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities capabilities;
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
+ wait_queue_head_t waitq;
+ struct fasync_struct *fasync;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -373,6 +394,158 @@ exit:
return rv;
}
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+ unsigned long arg)
+{
+ u8 *buffer;
+ struct device *dev;
+ int rv;
+ u8 tag, stb;
+
+ dev = >intf->dev;
+
+ dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+ data->iin_ep_present);
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+
+ atomic_set(>iin_data_valid, 0);
+
+ /* must issue read_stb before using poll or select */
+ atomic_set(>srq_asserted, 0);
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC488_REQUEST_READ_STATUS_BYTE,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ data->iin_bTag,
+ data->ifnum,
+ buffer, 0x03, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "control status returned %x\n",
+ buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (data->iin_ep_present) {
+
+ rv = wait_event_interruptible_timeout(
+ data->waitq,
+ (atomic_read(>iin_data_valid) != 0),
+ USBTMC_TIMEOUT
+ );
+
+ if (rv < 0) {
+ dev_dbg(dev, "wait interrupted %d\n", rv);
+ goto exit;
+ }
+
+ if (rv == 0) {
+ dev_dbg(dev, "wait timed out\n");
+ rv = -ETIME;
+ goto exit;
+ }
+
+ tag = data->bNotify1 & 0x7f;
+
+ if (tag != data->iin_bTag) {
+ dev_err(dev, "expected bTag %x got %x\n",
+ data->iin_bTag, tag);
+ }
+
+ stb = data->bNotify2;
+ } else {
+ stb = buffer[2];
+ }
+
+ /* bump interrupt bTag */
+ data->iin_bTag += 1;
+ if (data->iin_bTag > 127)
+ data->iin_bTag = 2;
+
+ rv = copy_to_user((void *)arg, , sizeof(stb));
+ if (rv)
+ rv = -EFAULT;
+
+ exit:
+ kfree(buffer);
+ return rv;
+
+}
+
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+ unsigned long arg,
+ unsigned int cmd)
+{
+ u8 *buffer;
+ struct device *dev;
+ int rv;
+ unsigned int val;
+ u16
[PATCH] xhci: fix warning when CONFIG_PM disabled.
From: Dave Hansen <dave.han...@linux.intel.com>
I have a .config with CONFIG_PM disabled. I get the following
whenever compiling the xhci driver:
drivers/usb/host/xhci-pci.c:192:13: warning: âxhci_pme_quirkâ defined but
not used [-Wunused-function]
Looks like we just need to move xhci_pme_quirk() to be underneath
the CONFIG_PM #ifdef.
Signed-off-by: Dave Hansen <dave.han...@linux.intel.com>
Cc: Mathias Nyman <mathias.ny...@intel.com> (supporter:USB XHCI DRIVER)
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org> (supporter:USB SUBSYSTEM)
Cc: linux-usb@vger.kernel.org (open list:USB XHCI DRIVER)
Cc: linux-ker...@vger.kernel.org (open list)
---
b/drivers/usb/host/xhci-pci.c | 90 +-
1 file changed, 45 insertions(+), 45 deletions(-)
diff -puN drivers/usb/host/xhci-pci.c~xhci-warning drivers/usb/host/xhci-pci.c
--- a/drivers/usb/host/xhci-pci.c~xhci-warning 2015-08-31 14:16:40.553236125
-0700
+++ b/drivers/usb/host/xhci-pci.c 2015-08-31 14:17:41.140001370 -0700
@@ -180,51 +180,6 @@ static void xhci_pci_quirks(struct devic
"QUIRK: Resetting on resume");
}
-/*
- * In some Intel xHCI controllers, in order to get D3 working,
- * through a vendor specific SSIC CONFIG register at offset 0x883c,
- * SSIC PORT need to be marked as "unused" before putting xHCI
- * into D3. After D3 exit, the SSIC port need to be marked as "used".
- * Without this change, xHCI might not enter D3 state.
- * Make sure PME works on some Intel xHCI controllers by writing 1 to clear
- * the Internal PME flag bit in vendor specific PMCTRL register at offset
0x80a4
- */
-static void xhci_pme_quirk(struct usb_hcd *hcd, bool suspend)
-{
- struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
- u32 val;
- void __iomem *reg;
-
- if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
-pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
-
- reg = (void __iomem *) xhci->cap_regs + PORT2_SSIC_CONFIG_REG2;
-
- /* Notify SSIC that SSIC profile programming is not done */
- val = readl(reg) & ~PROG_DONE;
- writel(val, reg);
-
- /* Mark SSIC port as unused(suspend) or used(resume) */
- val = readl(reg);
- if (suspend)
- val |= SSIC_PORT_UNUSED;
- else
- val &= ~SSIC_PORT_UNUSED;
- writel(val, reg);
-
- /* Notify SSIC that SSIC profile programming is done */
- val = readl(reg) | PROG_DONE;
- writel(val, reg);
- readl(reg);
- }
-
- reg = (void __iomem *) xhci->cap_regs + 0x80a4;
- val = readl(reg);
- writel(val | BIT(28), reg);
- readl(reg);
-}
-
#ifdef CONFIG_ACPI
static void xhci_pme_acpi_rtd3_enable(struct pci_dev *dev)
{
@@ -345,6 +300,51 @@ static void xhci_pci_remove(struct pci_d
}
#ifdef CONFIG_PM
+/*
+ * In some Intel xHCI controllers, in order to get D3 working,
+ * through a vendor specific SSIC CONFIG register at offset 0x883c,
+ * SSIC PORT need to be marked as "unused" before putting xHCI
+ * into D3. After D3 exit, the SSIC port need to be marked as "used".
+ * Without this change, xHCI might not enter D3 state.
+ * Make sure PME works on some Intel xHCI controllers by writing 1 to clear
+ * the Internal PME flag bit in vendor specific PMCTRL register at offset
0x80a4
+ */
+static void xhci_pme_quirk(struct usb_hcd *hcd, bool suspend)
+{
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+ u32 val;
+ void __iomem *reg;
+
+ if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
+
+ reg = (void __iomem *) xhci->cap_regs + PORT2_SSIC_CONFIG_REG2;
+
+ /* Notify SSIC that SSIC profile programming is not done */
+ val = readl(reg) & ~PROG_DONE;
+ writel(val, reg);
+
+ /* Mark SSIC port as unused(suspend) or used(resume) */
+ val = readl(reg);
+ if (suspend)
+ val |= SSIC_PORT_UNUSED;
+ else
+ val &= ~SSIC_PORT_UNUSED;
+ writel(val, reg);
+
+ /* Notify SSIC that SSIC profile programming is done */
+ val = readl(reg) | PROG_DONE;
+ writel(val, reg);
+ readl(reg);
+ }
+
+ reg = (void __iomem *) xhci->cap_regs + 0x80a4;
+ val = readl(reg);
+ writel(val | BIT(28), reg);
+ readl(reg);
+}
+
static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
{
stru
Re: Polling a usbfs file descriptor.
[quoted lines by Alan Stern on 2015/04/21 at 10:50 -0400] Yes, it's okay. A userspace ABI like that isn't going to go away. It's also true that you can poll for POLLERR or POLLHUP to see when a device has been disconnected, although libusb probably isn't interested in doing that. Out of curiosity, do you know why one must poll for output (rather than for input)? JDoes writability precisely mean that there's at least one user-submitted URB that can now be reaped? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Polling a usbfs file descriptor.
It seems (from inspecting libusb) that poll()ing a usbfs file descriptor for output can be used to determine when a urb can be reaped. A user has sent me a patch to our code that does this, and it actually does seem to work. So far, I haven't found any documentation on this. Is it okay for us to use this capability? Can anyone point me to documentation that formally describes how it works? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.org/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb gadget: remove size limitation for storage cdrom
On 03/09/15 at 10:22am, Alan Stern wrote:
On Mon, 9 Mar 2015, Dave Young wrote:
On 03/08/15 at 11:29am, Alan Stern wrote:
On Sun, 8 Mar 2015, Dave Young wrote:
I used usb cdrom emulation to play video dvd for my daughter, but I got
below
error:
[dave@darkstar tmp]$ cat /mnt/sr1/VIDEO_TS/VTS_01_5.VOB /dev/null
cat: /mnt/sr1/VIDEO_TS/VTS_01_5.VOB: Input/output error
[dave@darkstar tmp]$ dmesg|tail -1
[ 3349.371857] sr1: rw=0, want=8028824, limit=4607996
The assumption of cdrom size is not right, we can create data dvd large
then
4G, also mkisofs can create an iso with only one blank directory, the
size is
less than 300 sectors. The size limit does not make sense, thus remove
them.
Signed-off-by: Dave Young dyo...@redhat.com
---
drivers/usb/gadget/function/storage_common.c |9 -
1 file changed, 9 deletions(-)
--- linux.orig/drivers/usb/gadget/function/storage_common.c
+++ linux/drivers/usb/gadget/function/storage_common.c
@@ -247,15 +247,6 @@ int fsg_lun_open(struct fsg_lun *curlun,
num_sectors = size blkbits; /* File size in logic-block-size
blocks */
min_sectors = 1;
- if (curlun-cdrom) {
- min_sectors = 300; /* Smallest track is 300 frames
*/
- if (num_sectors = 256*60*75) {
- num_sectors = 256*60*75 - 1;
- LINFO(curlun, file too big: %s\n, filename);
- LINFO(curlun, using only first %d blocks\n,
- (int) num_sectors);
- }
- }
if (num_sectors min_sectors) {
LINFO(curlun, file too small: %s\n, filename);
rc = -ETOOSMALL;
NAK. This patch is wrong, for a very simple reason. You wrote:
I used usb cdrom emulation to play video dvd for my daughter
and this demonstrates the error quite plainly. You can't use _CD_
emulation to imitate a _DVD_ -- they are different sorts of media.
Just think of what happens when you try playing a DVD on a CD player.
A more suitable approach would be to add DVD emulation to the driver.
They are both iso9660 images, aren't they? So from data image point
of view there's no difference, it is not worth to create another mode
for dvd data.
There's more to emulation than just the image. We have to emulate the
hardware's response to all the applicable commands. CD players have a
different command set from DVD players (or at least, different from DVD
players when a DVD is inserted). For example, see the definition in
the MSF format for READ TOC command.
For data cd it works without those, but yes I will take a look how can
make MSF format ok. Possiblly it will need minimum commandset support.
As for audio cd/dvd, problem is there's no standard cd media format as a file
in filesystem. Meanwhile there's no much gain for the effort.
For dvd video, mplayer works happliy with size-limit-only fix though I can
not change subtile etc, I think full support dvd video need add extra scsi
command in code.
Long long ago I ever wrote a virtual cd driver as a block driver, but I lost
the code.
Will see if I can slowly rewrite one.
We should not emulate cd drive to support different cd media type,
it is far more better to support just iso9660 volume no matter how
large the size is as long as it is fit for iso9660 spec.
OTOH, the size limitation is a bug:
* isofs can be less than 300 sectors, the 300 sectors limitation is for
music cd I think. Try mkisofs a blank directory and burn it.
You're thinking about this the wrong way. We aren't emulating an
iso9660 image; we are emulating a CD player. (In principle we could
even emulate an audio CD, but the driver doesn't support that.)
Right, but data iso is the most of use cases. For virtual cd drive we can
choose to support data cd only or data cd first.
* There's 99 minutes dics even for cdrom, see:
http://en.wikipedia.org/wiki/CD-R
If we code to support different size discs, it will looks like a wrong way.
Look at the code you want to remove:
- if (num_sectors = 256*60*75) {
That's 75 sectors/second * 60 seconds/minute * 256 minutes. So the
driver already supports up to 256 minutes, which is way beyond the 99
minutes required by the spec.
Oops, I missed the line, so as I said I will take a look at how to fix it
both msf and *lba* mode.
But I have limited time, will response slow and play with it at weekend.
Thanks
Dave
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb gadget: remove size limitation for storage cdrom
On 03/08/15 at 11:29am, Alan Stern wrote:
On Sun, 8 Mar 2015, Dave Young wrote:
I used usb cdrom emulation to play video dvd for my daughter, but I got
below
error:
[dave@darkstar tmp]$ cat /mnt/sr1/VIDEO_TS/VTS_01_5.VOB /dev/null
cat: /mnt/sr1/VIDEO_TS/VTS_01_5.VOB: Input/output error
[dave@darkstar tmp]$ dmesg|tail -1
[ 3349.371857] sr1: rw=0, want=8028824, limit=4607996
The assumption of cdrom size is not right, we can create data dvd large then
4G, also mkisofs can create an iso with only one blank directory, the size
is
less than 300 sectors. The size limit does not make sense, thus remove
them.
Signed-off-by: Dave Young dyo...@redhat.com
---
drivers/usb/gadget/function/storage_common.c |9 -
1 file changed, 9 deletions(-)
--- linux.orig/drivers/usb/gadget/function/storage_common.c
+++ linux/drivers/usb/gadget/function/storage_common.c
@@ -247,15 +247,6 @@ int fsg_lun_open(struct fsg_lun *curlun,
num_sectors = size blkbits; /* File size in logic-block-size blocks
*/
min_sectors = 1;
- if (curlun-cdrom) {
- min_sectors = 300; /* Smallest track is 300 frames */
- if (num_sectors = 256*60*75) {
- num_sectors = 256*60*75 - 1;
- LINFO(curlun, file too big: %s\n, filename);
- LINFO(curlun, using only first %d blocks\n,
- (int) num_sectors);
- }
- }
if (num_sectors min_sectors) {
LINFO(curlun, file too small: %s\n, filename);
rc = -ETOOSMALL;
NAK. This patch is wrong, for a very simple reason. You wrote:
I used usb cdrom emulation to play video dvd for my daughter
and this demonstrates the error quite plainly. You can't use _CD_
emulation to imitate a _DVD_ -- they are different sorts of media.
Just think of what happens when you try playing a DVD on a CD player.
A more suitable approach would be to add DVD emulation to the driver.
They are both iso9660 images, aren't they? So from data image point
of view there's no difference, it is not worth to create another mode
for dvd data.
We should not emulate cd drive to support different cd media type,
it is far more better to support just iso9660 volume no matter how
large the size is as long as it is fit for iso9660 spec.
OTOH, the size limitation is a bug:
* isofs can be less than 300 sectors, the 300 sectors limitation is for
music cd I think. Try mkisofs a blank directory and burn it.
* There's 99 minutes dics even for cdrom, see:
http://en.wikipedia.org/wiki/CD-R
If we code to support different size discs, it will looks like a wrong way.
Thanks
Dave
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb gadget: remove size limitation for storage cdrom
I used usb cdrom emulation to play video dvd for my daughter, but I got below
error:
[dave@darkstar tmp]$ cat /mnt/sr1/VIDEO_TS/VTS_01_5.VOB /dev/null
cat: /mnt/sr1/VIDEO_TS/VTS_01_5.VOB: Input/output error
[dave@darkstar tmp]$ dmesg|tail -1
[ 3349.371857] sr1: rw=0, want=8028824, limit=4607996
The assumption of cdrom size is not right, we can create data dvd large then
4G, also mkisofs can create an iso with only one blank directory, the size is
less than 300 sectors. The size limit does not make sense, thus remove them.
Signed-off-by: Dave Young dyo...@redhat.com
---
drivers/usb/gadget/function/storage_common.c |9 -
1 file changed, 9 deletions(-)
--- linux.orig/drivers/usb/gadget/function/storage_common.c
+++ linux/drivers/usb/gadget/function/storage_common.c
@@ -247,15 +247,6 @@ int fsg_lun_open(struct fsg_lun *curlun,
num_sectors = size blkbits; /* File size in logic-block-size blocks
*/
min_sectors = 1;
- if (curlun-cdrom) {
- min_sectors = 300; /* Smallest track is 300 frames */
- if (num_sectors = 256*60*75) {
- num_sectors = 256*60*75 - 1;
- LINFO(curlun, file too big: %s\n, filename);
- LINFO(curlun, using only first %d blocks\n,
- (int) num_sectors);
- }
- }
if (num_sectors min_sectors) {
LINFO(curlun, file too small: %s\n, filename);
rc = -ETOOSMALL;
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
[quoted lines by Alan Stern on 2015/02/12 at 10:44 -0500] Doesn't your program mask those signals before submitting URBs? It must; otherwise the completion signals would be delivered to the program in the normal way via a signal handler instead of being reported via signalfd. Once the last monitor for a given signal is removed, the former signal blocking state for that signal is restored. That being said, yes, I'd forgotten that way back at program startup (when there's only one thread running), to prevent those signals from going to the wrong thread, all of them are arbitrarily blocked. That lets each thread safely unblock each signal it cares about. So, I aghree, there's no bug - just a bit of memory loss on my part. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
[quoted lines by Alan Stern on 2015/02/12 at 10:10 -0500] Not to me. The purpose of signalfd is to report pending signals. Closing or opening a file descriptor shouldn't change the set of pending signals. Once the last signalfd file descriptor for a given signal is closed - in our case, that's only one of them - shouldn't the handling revert to the default for that signal? Isn't it wrong that, when there are no signalfd file descriptors open for a given signal, the signal still seems to stay pending within the signalfd mechanism? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
[quoted lines by Alan Stern on 2015/02/11 at 10:13 -0500] What do you mean by turned off? When the device is turned off, does it appear to the host controller that the device has disconnected from the USB bus? No. This also occurs after connecting the device while it's turned off. In this state, the device is connected. When, with its switch in the off position, I connect it, my log shows: kernel: [6125541.188647] usb 5-3: New USB device found, idVendor=10c4, idProduct=ea80 kernel: [6125541.188659] usb 5-3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 kernel: [6125541.188667] usb 5-3: Product: CP2110 HID USB-to-UART Bridge kernel: [6125541.188673] usb 5-3: Manufacturer: Silicon Laboratories kernel: [6125541.188679] usb 5-3: SerialNumber: 0012EC89 kernel: [6125541.287283] hid-generic 0003:10C4:EA80.0011: hiddev0,hidraw2: USB HID v1.11 Device [Silicon Laboratories CP2110 HID USB-to-UART Bridge] on usb-:00:13.0-3/input0 mtp-probe: checking bus 5, device 12: /sys/devices/pci:00/:00:13.0/usb5/5-3 mtp-probe: bus: 5, device: 12 was not an MTP device Which signal? Is this the signal for URB completion or the signal for device disconnection? Completion. urb-signr is set to 34 (SIGRTMIN), and that's the signal I'm getting. Did you turn off the device while the URB was in progress, or was the device turned off all along? It was turned off all along. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
[quoted lines by Alan Stern on 2015/02/11 at 11:13 -0500] Hmmm. In addition to usbmon, you can try writing 1 to /sys/module/usbcore/parameters/usbfs_snoop and then seeing what shows up in the dmesg log. Looking at the snoop logs is hinting that I may need to add another piece to this puzzle. It's showing that the urb doesn't complete until the device is finally closed. This makes sense because, being turned off, the device isn't responding to the probes. That's also what I used to see when using an actual signal handler. What I didn't mention (because I didn't think it'd be significant) is that I've started seeing the current problem when I switched to using signalfd so that a signal handler wouldn't be necessary. What I see when using signalfd is that the signal arrives (the signalfd file descriptor becomes readable) almost immediately, i.e. as soon as the poll() is entered to wait for a response from the first probe. Reading the struct signalfd_siginfo from the signalfd file descriptor does give the expected signal number. So now, perhaps the question is could it be that signalfd data is arriving too early? Could signalfd and signal handler behaviour really be different like that? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
[quoted lines by Peter Stuge on 2015/02/11 at 16:40 +0100] But I don't believe you should have to discard the URB when the device disappears. I can see, from Alan's questions, that I wasn't clear enough. By was turned off, I meant that the device's power switch was in its off position. It was in that state all the way through. It wasn't turned off part way through. Even in this state, the device is connected - just not fully operational. So what we have is that the urb can be discareded, which mdans that it's for sure there, but that a non-blocking reap yields EAGAIN and a blocking reap waits forever. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
[quoted lines by Alan Stern on 2015/02/11 at 12:11 -0500] That probably is the explanation. I don't know anything about signalfd; you should ask somebody who does. Okay, I shall. Perhaps you could answer another question that I've been wondering about. Does usbfs send the completion signal to the thread that submitted the urb or to the whole process? If to the whole process, is there a way to redirect it to just the thread? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
Thanks. We do take care to block the signal for all other threads just in case. Regarding the possibility of signalfd delivering the signal too early: Wouldn't it still be true that something within the usbfs code would still need to be doing something to trigger that behaviour? If usbfs really never does anything to deliver the completion signal until a urb has actually completed then what might it be doing to trigger a signalfd notificatoin earlier than that? -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
Some information that may be helpful: There's a significant difference in the way a signal handler is used versus the way signalfd notifications are used. That difference is that the signal is unblocked when a signal handler is used (so that the handler can catch it) but is blocked when signalfd notifications are used (so that a handler won't catch it). This difference offers a possible explanation. Is it possible that usbfs sets up the signal right away, but blocks it until the URB completes? If so, the signal would remain pending and a signal handler would get it at the right time whereas a signalfd notification would occur immediately. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
[quoted lines by Alan Stern on 2015/02/11 at 10:54 -0500] I just reverified. It's definitely returning -1 with EAGAIN. That indicates the device is connected and the URB is still in progress. You can't reap it because it hasn't completed. The signal you received must have referred to a different URB. there's only one urb. The code actually verifies this by comparing the address of the reaped urb to the one that was submitted. In this case, however, reaping is failibng so it doesn't get that far. Could the signal be arriving too early? If I give it the address of the urb I'm expecting, it returns 0, and then the non-blocking reap changes to returning -1 with ENOENT. Because when you cancel the URB, it completes. Which also would seem to verify that there's only that one urb. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Unable to reap urb after receiving signal using usbfs.
I'd appreciate some advice from anyone here who's familiar with using signal-based notification (setting urb-signr) when using usbfs. It's working correctly when this particular device is on, as well as when it's disconnected, but I'm seeing what seems to be odd behaviour when the device is connected but off. Since querying the configuration and claiming the intterface both work, the code goes ahead and tries to do I/O. The URB for expecting input is submitted, the signal arrives, but the non-blocking reap fails with EAGAIN. I tried a blocking reap, which ended up waiting forever. My question, therefore, is: Under what circumstances should usbfs code, after having received a signal, expect to not be able to reap a URB, and how should it interpret this situation? In case it matters: It's an interrupt input endpoint. The error indicated by the signal (siginfo-si_errno) is -ENOENT, which, as I see it, does make sense because, with the device connected but off, it could mean that the USB descriptors are available but that the endpoints can't actually be used. In this situation, though, wouldn't the URB still be reapable with its status set to -ENOENT? I see the following code in drivers/usb/core/devio.c, that does seem to be doing some kind of urb cancellation: 518: if (as-status 0 as-bulk_addr as-status != -ECONNRESET 519: as-status != -ENOENT) 520: cancel_bulk_urbs(ps, as-bulk_addr); Whatever this code is for, it looks like it's specifically not being executed if the error is -ENOENT. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to reap urb after receiving signal using usbfs.
[quoted lines by Peter Stuge on 2015/02/11 at 15:57 +0100] I would expect ioctl(fd, IOCTL_USBFS_REAPURBNDELAY, urb) to either return -1 with errno = ENODEV, or to return 0 with urb-status = ESHUTDOWN. I just reverified. It's definitely returning -1 with EAGAIN. What happens if you ioctl(fd, IOCTL_USBFS_DISCARDURB, ) instead of REAP? If I give it the address of the urb I'm expecting, it returns 0, and then the non-blocking reap changes to returning -1 with ENOENT. -- Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God. Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/ EMail: d...@mielke.cc | Canada K2A 1H7 | http://FamilyRadio.com/ -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
