NetBSD Problem Report #47179

From www@NetBSD.org  Sat Nov 10 22:40:54 2012
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id 6BED563E6ED
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 10 Nov 2012 22:40:54 +0000 (UTC)
Message-Id: <20121110224053.9B30463E6ED@www.NetBSD.org>
Date: Sat, 10 Nov 2012 22:40:53 +0000 (UTC)
From: darkstar@city-net.com
Reply-To: darkstar@city-net.com
To: gnats-bugs@NetBSD.org
Subject: major issues with i2c locking (iic_acquire_bus/iic_release_bus)
X-Send-Pr-Version: www-1.0

>Number:         47179
>Category:       kern
>Synopsis:       major issues with i2c locking (iic_acquire_bus/iic_release_bus)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Nov 10 22:45:00 +0000 2012
>Originator:     Matthew Orgass
>Release:        6.99.10
>Organization:
>Environment:
examination of source code
>Description:
Between the controller drivers and device drivers many types of errors in iic_acquire_bus and iic_release_bus usage are represented and few combinations are correct. I2C_F_POLL might always be handled incorrectly.

The return value of iic_acquire_bus is not documented in iic(9).  

The actual return values are inconsistent and sometimes contradictory between controllers.  

Some controller drivers seem to do something related to preparing the bus to be accessed in iic_acquire_bus but do not actually lock anything.  

Many controller drivers always use mutex_lock in iic_acquire_bus without checking for I2C_F_POLL.

Some controller drivers always return what they think means failure if I2C_F_POLL is specified to iic_acquire_bus.

Many device drivers do not check the return value of iic_acquire_bus even if they specified I2C_F_POLL.  Sometimes there is a layer of indirection that returns the return value, which is then usually ignored by callers.

Some drivers have indirected read/write functions which lock the bus, read or write, and then release the bus.  These functions are usually called many times in series, rather than locking the bus once and then reading and writing as needed and releasing when done.

The return value of iic_release_bus is documented as int but it looks like every controller driver returns void because that makes sense.  


>How-To-Repeat:
examine source code
>Fix:
I'll eventually submit a patch, but it is taking a while so I thought I would send-pr the problem so it isn't lost.  If someone wants to get to it faster, go for it.

Basically, all usage of iic_acquire_bus and iic_release_bus in both controller and device drivers needs to be checked and fixed and the documentation updated.  It seems like the return value of iic_acquire_bus should maybe be the return value of mutex_tryenter (although this is contrary to what seems to be the most common assumption now) and that it should be documented to always succeed if I2C_F_POLL is not specified.  An iic generic macro or function should probably be provided to DTRT in controller iic_acquire_bus functions.  Possibly, the mutex could just be put in struct i2c_controller and handled in a completely controller independent way, although that would be the only regularly modified data in that structure and a few controllers seem to want hooks at those points anyway.

Home	PR Database Search