Re: [PATCH 03/15] ACPICA: move common private headers under kernel/acpi/acpica/

[Ingo Molnar <mingo@elte.hu>]

* Len Brown <lenb@kernel.org> wrote:

> From: Len Brown <len.brown@intel.com>
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---

>  kernel/acpi/acpica/dsfield.c                    |    2 +-
>  kernel/acpi/acpica/dsinit.c                     |    2 +-
>  kernel/acpi/acpica/dsmethod.c                   |    2 +-
>  kernel/acpi/acpica/dsmthdat.c                   |    2 +-
>  kernel/acpi/acpica/dsobject.c                   |    2 +-
>  kernel/acpi/acpica/dsopcode.c                   |    2 +-
>  kernel/acpi/acpica/dsutils.c                    |    2 +-
>  kernel/acpi/acpica/dswexec.c                    |    2 +-
>  kernel/acpi/acpica/dswload.c                    |    2 +-
>  kernel/acpi/acpica/dswscope.c                   |    2 +-
>  kernel/acpi/acpica/dswstate.c                   |    2 +-
>  kernel/acpi/acpica/evevent.c                    |    2 +-
>  kernel/acpi/acpica/evgpe.c                      |    2 +-
>  kernel/acpi/acpica/evgpeblk.c                   |    2 +-
>  kernel/acpi/acpica/evmisc.c                     |    2 +-
>  kernel/acpi/acpica/evregion.c                   |    2 +-
>  kernel/acpi/acpica/evrgnini.c                   |    2 +-
>  kernel/acpi/acpica/evsci.c                      |    2 +-
>  kernel/acpi/acpica/evxface.c                    |    2 +-
>  kernel/acpi/acpica/evxfevnt.c                   |    2 +-
>  kernel/acpi/acpica/evxfregn.c                   |    2 +-
>  kernel/acpi/acpica/exconfig.c                   |    2 +-
>  kernel/acpi/acpica/exconvrt.c                   |    2 +-
>  kernel/acpi/acpica/excreate.c                   |    2 +-
>  kernel/acpi/acpica/exdump.c                     |    2 +-
>  kernel/acpi/acpica/exfield.c                    |    2 +-
>
>  [... etc ...]

hm, dunno. Do we really want to introduce 'driver/platform' space items 
like this in the core kernel/* ?

If it goes there then IMHO the ACPI code needs to be cleaned up 
_significantly_ to not wrap native Linux calls like spinlocks, allocators, 
etc.

Random example - i dont think stuff like this is readable [in to-be 
kernel/acpi/utilities/utcache.c]:

        if (cache->current_depth >= cache->max_depth) {
                ACPI_FREE(object);
                ACPI_MEM_TRACKING(cache->total_freed++);
        }

        /* Otherwise put this object back into the cache */

        else {
                status = acpi_ut_acquire_mutex(ACPI_MTX_CACHES);
                if (ACPI_FAILURE(status)) {
                        return (status);
                }

acpi_ut_acquire_mutex() under [to-be] kernel/acpi/utilities/utmutex.c 
looks absolutely horrible, redirected after some debugging layer to its 
final destination:

./include/acpi/acpiosxf.h:#define acpi_os_acquire_mutex(handle,time) 
acpi_os_wait_semaphore (handle, 1, time), which does [kernel/acpi/osl.c]:

/*
 * TODO: Support for units > 1?
 */
acpi_status acpi_os_wait_semaphore(acpi_handle handle, u32 units, u16 timeout)
{
	acpi_status status = AE_OK;
	struct semaphore *sem = (struct semaphore *)handle;
	long jiffies;
	int ret = 0;

	if (!sem || (units < 1))
		return AE_BAD_PARAMETER;

	if (units > 1)
		return AE_SUPPORT;

	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Waiting for semaphore[%p|%d|%d]\n",
			  handle, units, timeout));

	if (timeout == ACPI_WAIT_FOREVER)
		jiffies = MAX_SCHEDULE_TIMEOUT;
	else
		jiffies = msecs_to_jiffies(timeout);
	
	ret = down_timeout(sem, jiffies);
	if (ret)
		status = AE_TIME;

	if (ACPI_FAILURE(status)) {
		ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
				  "Failed to acquire semaphore[%p|%d|%d], %s",
				  handle, units, timeout,
				  acpi_format_exception(status)));
	} else {
		ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
				  "Acquired semaphore[%p|%d|%d]", handle,
				  units, timeout));
	}

	return status;
}

so it's a glorified down_timeout(). While we have mutex_timeout(). What's 
the plan here?

	Ingo