VME userland API

Martyn Welch martyn.welch at ge.com
Wed Jul 25 11:15:21 UTC 2012


On 25/07/12 10:20, Wolfgang Mauerer wrote:
> On 24/07/12 23:53, Martyn Welch wrote:
>> On 24/07/12 16:34, Wolfgang Mauerer wrote:
>>> On 20/07/12 10:48, Martyn Welch wrote:
>>>> On 19/07/12 23:39, Greg Kroah-Hartman wrote:
>>>>> On Thu, Jul 19, 2012 at 08:08:50PM +0100, Martyn Welch wrote:
>>>
>>>> Still in two minds about whether we should keep vme_user around. It does seem
>>>> to have users and there are simple drivers/tasks that could be coded in user
>>>> space using this. On the other hand, the interface to user space it currently
>>>> provides isn't idea, neither is the fact that it grabs 4 VME windows when the
>>>> module is loaded, regardless of whether they ever get used. It was certainly
>>>> useful during development of the VME subsystem, but my focus is really on the
>>>> subsystem and kernel space VME drivers.
>>>
>>> vme_user is useful when it comes to porting legacy VME code
>>> to Linux. Having a possibility to access the bus from userland will
>>> surely ease this task, userland is simply more convenient to work
>>> with. If speed is not too important, there's no real need to
>>> involve all the kernel complexity.
>>>
>>> Besides the sub-optimal interface, there are some functionalities that 
>>> should be exported in addition to what is currently available from
>>> vme_user. I'm working on cleaning up the driver, and would suggest to 
>>> adapt the interface as follows:
>>>
>>> - Support more than one bridge; place the device files in
>>>   /dev/bus/vme/000/... etc., similar to the USB subsystem
>>
>> Definitely.
>>
>>> - Dynamically query the number of available master/slave windows
>>>   from the low-level driver, expose this information to userland,
>>>   and don't create access windows on startup, but only when needed.
>>
>> The driver definitely needs to stop grabbing windows at load. The
>> framework expects the drivers to request resources when required, rather
>> than querying the number of available windows, I'd prefer that the user
>> space driver/application request a window, with some mechanism to tell
>> whether the request had succeeded. I'm not sure there's any point in a
> okay.
>> function that tells you how many are at any given point in tie, that
>> information could be wildly wrong by the time a request for one is made.
>> The request would probably need to include the characteristics required
>> as not all windows are equal (only some slave windows can do A16 in
>> certain chipsets, some chipsets can't do A64 at all, etc).
> knowing the maximum number of windows is indeed not strictly required,
> but it might still make sense to export these properties via sysfs.
> Userspace could want this to use different strategies (e.g., don't
> even try to allocate 6 windows if the driver support only 4).
> 

Which is fine, however this assumes that only one process will be requesting
windows from user space. If kernel space drivers have grabbed some and/or
there are more than one process using VME windows in user space, knowing the
total number of windows or the number at any given point in time becomes a
little meaningless, as the number actually available could have changed by the
point your program actually attempts to make a request. Effectively, if your
program wants 2 windows, it will need to request these when it starts, there's
no guarantee that they will be available at a later date. If it can get by
with one, then a failed request for the second window at start should be
treated by the application as non-fatal.

> Btw., what was the rationale behind the comment
> 
> /* XXX  We do not want to push aspace, cycle and width
>  *      to userspace as they are
>  */
> 
> that appears several times in vme_user? I don't see any
> obvious reason why userspace should not know these properties,
> but maybe I'm missing something.
>

I think the reason for this is highlighted in the discussion further down.

>>> - Expose the RMW mechanism and semaphores to userland
>>> - Implement a timeout mechanism when waiting for IACKs
>>
>> That would need to be implemented in the drivers, then exposed through
>> the kernel API.
>>
>>> - Provide generic/status information (e.g., sysfail, slot id, ...), via
>>>   some mechnism (ioctl, sysfs, whatever)
>>> - Possibly map location monitors into userland (interrupt proagation
>>>   via uio)
>>>
>>> Any comments on these ideas?
>>>
>>> A pull request for some changes in this direction is given below.
>>> However, the code is _NOT_ meant to be pulled right now -- the patches 
>>> are not yet in shape; some things still need to be implemented and/or 
>>> polished. But it could simplify the discussion.
>>>
>>> Unfortunately, I cannot do any real tests for a temporary lack of 
>>> hardware at the moment. But this should not really concern a general 
>>> discussion about the future shape of this driver.
>>>
>>> Best, Wolfgang
>>>
>>> ####################################################################
>>> The following changes since commit 28a33cbc24e4256c143dce96c7d93bf423229f92:                                      
>>>
>>>   Linux 3.5 (2012-07-21 13:58:29 -0700)
>>>
>>> are available in the git repository at:
>>>   git://github.com/siemens/vme.git not-yet-for-upstream
>>>
>>
>> Few comments below:
>>
>>> Wolfgang Mauerer (16):
>>>       Tiny comment fix
>>>       Extract VME major number definition from the device driver
>>>       vme_user: Clarify buffer allocation comment
>>>       vme_user: Get rid of Universe II references
>>>       vme_user: Cosmetic fixes
>>>       vme_user: Some more context for error messages
>>
>> Use pr_err and friends?
> sure. Luckily, there's a patch on the ML to do exactly this ;)
>>
>>>       vme_user: Indentation cleanups
>>
>> Think there may be a few more :-)
> agreed... I'll add a final patch to correct the issues after
> the series is finished.
>>
>>>       vme_user: Push ioctl arguments into local scope
>>
>> This patch removes a "break;", I might be wrong (just looking at the
>> patch in isolation) but isn't it required?
> that could well be the case. A later patch reworks the state machine and
> inserts breaks everywhere, I'll have to squash these patches together.
>>
>>>       vme_user: Add support for sending RMW cycles
>>>       vme: Equip irq generation with a timeout
>>
>> I appreciate that you won't have access to all the bridges, but I think
>> the ca91cx42 driver needs to be changed at least test to see if a
>> timeout has been provided and error/throw a warning if it has.
> okay.
>>
>>>       vme_user: Use device_type methods to map per-bridge information into userland
>>
>> The function to retrieve the bus enumeration is very similar to part of
>> the patch I proposed earlier in this thread (I think you need to check
>> the pointers not null).
> I'll use your change in later revisions.
>>
>> The device model bits should probably be in the core rather than
>> vme_user, wouldn't this functionality make sense for all drivers?
> okay.
>>
>>>       vme_user: Export slot id to userland
>>
>> Looks good (as long as we are going the IOCTL route).
>>
>>>       tsi148: Allow for exporting the bridge status
>>
>> Looks Ok.
>>
>>>       ca91c42x: Allow for exporting status information
>>
>> Don't add the function if it's not supported - have the framework just
>> return -EINVAL if the pointer is null (which it already does)
> the reason there's a nop function here is that I don't know if ca91c42x
> can expose these status bits -- perhaps any of the experts on this
> chipset can help out.
> 
> As I go along, I may be adding other pieces of information that are also
> supported by ca91c42x, so I'll keep the function for the time being
> (if it turns out there's really nothing that can be exported from this
> bridge, the function will be removed)
> 

No, just don't add it. It can be added at a later date if it gets supported.

Looking at the data sheet quickly suggests that the ca91c42x does provide
support for ACFAIL and SYSFAIL. It would appear the both the tsi148 and
ca91c42x provide interrupt support on these lines as well, so a simple polling
method to read the status of these lines many not be the best way to go. It
would seem that, these signals are likely to be asynchronous in nature and
especially with ACFAIL will need to be actioned promptly.

What do you think about supporting these (at the kernel API level) as
interrupts with callbacks? This could be exposed to user space in vme_user as
a signal on assertion of the relevant line or vme_user could expose this as a
polled interface (inactive until callback triggered).

>>
>>>       vme, vme_user: Make bridge status public
>>
>> Either the status structure should be in vme.h (so as not to need
>> vme_user.h in the core) or the sysfail and acfail should be retrievable
>> without the structure and the structure is a vme_user only thing.
> if it's okay to put it into vme.h, I'm fine with that. But since it's
> shared information between kernel and userland, this would require
> to make vme.h available for userland, which it is currently
> not (vme_user would need need to be marked as header-y anyway).
> 

Hmm, I don't think vme.h should be exposed to user space. This was probably
the rationale for the "We do not want to push aspace, cycle and width to
userspace as they are" message. IIRC, vme_user currently uses the definitions
in vme.h in it's structures to enumerate these attributes.

> We could also create a separate file with shared definitions that
> is included by vme.h and vme_user.h. Any preferences?

Pulling out the definitions (address spaces, cycle types and data widths) near
the top of vme.h into a separate shared header file probably makes a lot of
sense and would mean that we could get rid of the aforementioned messages.

Martyn

>>
>>>       vme_user: Simplify ioctl state machine
>>
>> I think the pr_fmt added here would be of benefit in the "vme_user: Some
>> more context for error messages" patch.
> yeah, need to squash the patches appropriately.
> 
>>
>> There's definitely some good stuff here, look forward to seeing them
>> polished :-)
> 
> Thanks, Wolfgang
> 


-- 
Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
T +44(0)1327322748                     | Manchester, M2 3AB
E martyn.welch at ge.com                  | VAT:GB 927559189



More information about the devel mailing list