pci-acpi: improvement for _OSC evaluation
cover pci-acpi: improvement for _OSC evaluation
Hi,
Here are some improvement patches for _OSC evaluation on PCI.
- [PATCH 1/5] pci-acpi: remove duplicate code for _OSC
- [PATCH 2/5] pci-acpi: use local buffer for _OSC
- [PATCH 3/5] pci-acpi: add flag to indicate query had been done
- [PATCH 4/5] pci-acpi: remove unused variable in __pci_osc_support_set
- [PATCH 5/5] pci-acpi: minor cleanups for _OSC
For details, please see the header of each patch.
Thanks,
Kenji Kaneshige
PATCH 2/5 pci-acpi: use local buffer for _OSC
Current pci-acpi implementation uses array in osc_data directly to evaluate _OSC. It needs to save the old data and restore it if _OSC evaluation fails. To make it more robust, we should use local array to evaluate _OSC.
--- drivers/pci/pci-acpi.c | 73 ++++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 34 deletions(-)
drivers/pci/pci-acpi.c +39 -34
@@ -21,12 +21,18 @@struct acpi_osc_data {acpi_handle handle; -u32 ctrlset_buf[3 ]; -u32 global_ctrlsets; +u32 support_set; +u32 control_set; +u32 query_result;struct list_head sibiling; };static LIST_HEAD (acpi_osc_data_list); +struct acpi_osc_args { +u32 capbuf[3 ]; +u32 query_result; +}; +static struct acpi_osc_data *acpi_get_osc_data (acpi_handle handle) {struct acpi_osc_data *data;
@@ -47,14 +53,14 @@ static struct acpi_osc_data *acpi_get_osstatic u8 OSC_UUID [16 ] = {0x5B ,0x4D ,0xDB ,0x33 ,0xF7 ,0x1F ,0x1C ,0x40 ,0x96 ,0x57 ,0x74 ,0x41 ,0xC0 ,0x3D ,0xD7 ,0x66 };static acpi_status acpi_run_osc (acpi_handle handle, -struct acpi_osc_data *osc_data) +struct acpi_osc_args *osc_args) {acpi_status status;struct acpi_object_list input;union acpi_object in_params[4 ];struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER ,NULL };union acpi_object *out_obj; -u32 osc_dw0, flags = osc_data->ctrlset_buf[OSC_QUERY_TYPE ]; +u32 osc_dw0, flags = osc_args->capbuf[OSC_QUERY_TYPE ];/* Setting up input parameters */ input.count =4 ;
@@ -68,7 +74,7 @@ static acpi_status acpi_run_osc(acpi_han in_params[2 ].integer.value =3 ; in_params[3 ].type =ACPI_TYPE_BUFFER ; in_params[3 ].buffer.length =12 ; - in_params[3 ].buffer.pointer = (u8 *)osc_data->ctrlset_buf; + in_params[3 ].buffer.pointer = (u8 *)osc_args->capbuf; status =acpi_evaluate_object (handle,"_OSC" , &input, &output);if (ACPI_FAILURE (status))
@@ -99,11 +105,9 @@ static acpi_status acpi_run_osc(acpi_han goto out_kfree; } out_success: -if (flags &OSC_QUERY_ENABLE ) { -/* Update Global Control Set */ - osc_data->global_ctrlsets = +if (flags &OSC_QUERY_ENABLE ) + osc_args->query_result = *((u32 *)(out_obj->buffer.pointer +8 )); - } status =AE_OK ; out_kfree:
@@ -117,8 +121,9 @@ static acpi_status acpi_query_osc(acpi_hacpi_status status;acpi_status *ret_status = (acpi_status *)retval;struct acpi_osc_data *osc_data; -u32 flags = (unsigned long )context, temp; +u32 flags = (unsigned long )context, support_set;acpi_handle tmp; +struct acpi_osc_args osc_args; status =acpi_get_handle (handle,"_OSC" , &tmp);if (ACPI_FAILURE (status))
@@ -130,21 +135,18 @@ static acpi_status acpi_query_osc(acpi_hreturn AE_ERROR ; } - osc_data->ctrlset_buf[OSC_SUPPORT_TYPE ] |= (flags &OSC_SUPPORT_MASKS ); -/* do _OSC query for all possible controls */ - temp = osc_data->ctrlset_buf[OSC_CONTROL_TYPE ]; - osc_data->ctrlset_buf[OSC_QUERY_TYPE ] =OSC_QUERY_ENABLE ; - osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] =OSC_CONTROL_MASKS ; + support_set = osc_data->support_set | (flags &OSC_SUPPORT_MASKS ); + osc_args.capbuf[OSC_QUERY_TYPE ] =OSC_QUERY_ENABLE ; + osc_args.capbuf[OSC_SUPPORT_TYPE ] = support_set; + osc_args.capbuf[OSC_CONTROL_TYPE ] =OSC_CONTROL_MASKS ; - status =acpi_run_osc (handle, osc_data); + status =acpi_run_osc (handle, &osc_args); *ret_status = status; - osc_data->ctrlset_buf[OSC_QUERY_TYPE ] = !OSC_QUERY_ENABLE ; - osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] = temp; -if (ACPI_FAILURE (status)) { -/* no osc support at all */ - osc_data->ctrlset_buf[OSC_SUPPORT_TYPE ] =0 ; +if (ACPI_SUCCESS (status)) { + osc_data->support_set = support_set; + osc_data->query_result = osc_args.query_result; }return status;
@@ -181,10 +183,11 @@ acpi_status __pci_osc_support_set(u32 fl**/ acpi_status pci_osc_control_set (acpi_handle handle,u32 flags) { -acpi_status status; -u32 ctrlset; +acpi_status status; +u32 ctrlset, control_set;acpi_handle tmp;struct acpi_osc_data *osc_data; +struct acpi_osc_args osc_args; status =acpi_get_handle (handle,"_OSC" , &tmp);if (ACPI_FAILURE (status))
@@ -197,19 +200,21 @@ acpi_status pci_osc_control_set(acpi_han } ctrlset = (flags &OSC_CONTROL_MASKS ); -if (!ctrlset) { +if (!ctrlset)return AE_TYPE ; - } -if (osc_data->ctrlset_buf[OSC_SUPPORT_TYPE ] && - ((osc_data->global_ctrlsets & ctrlset) != ctrlset)) { + +if (osc_data->support_set && + ((osc_data->query_result & ctrlset) != ctrlset))return AE_SUPPORT ; - } - osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] |= ctrlset; - status =acpi_run_osc (handle, osc_data); -if (ACPI_FAILURE (status)) { - osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] &= ~ctrlset; - } - + + control_set = osc_data->control_set | ctrlset; + osc_args.capbuf[OSC_QUERY_TYPE ] =0 ; + osc_args.capbuf[OSC_SUPPORT_TYPE ] = osc_data->support_set; + osc_args.capbuf[OSC_CONTROL_TYPE ] = control_set; + status =acpi_run_osc (handle, &osc_args); +if (ACPI_SUCCESS (status)) + osc_data->control_set = control_set; +return status; }EXPORT_SYMBOL (pci_osc_control_set);
PATCH 1/5 pci-acpi: remove duplicate code for _OSC
In reply to Kenji KaneshigeRemove the duplicated code in acpi_query_osc() and acpi_run_osc(). It simplifies the code very much.
--- drivers/pci/pci-acpi.c | 165 ++++++++++++++++--------------------------------- 1 file changed, 55 insertions(+), 110 deletions(-)
drivers/pci/pci-acpi.c +55 -110
@@ -46,40 +46,15 @@ static struct acpi_osc_data *acpi_get_osstatic u8 OSC_UUID [16 ] = {0x5B ,0x4D ,0xDB ,0x33 ,0xF7 ,0x1F ,0x1C ,0x40 ,0x96 ,0x57 ,0x74 ,0x41 ,0xC0 ,0x3D ,0xD7 ,0x66 }; -static acpi_status -acpi_query_osc ( -acpi_handle handle, -u32 level, -void *context, -void **retval ) +static acpi_status acpi_run_osc (acpi_handle handle, +struct acpi_osc_data *osc_data) { -acpi_status status; -struct acpi_object_list input; -union acpi_object in_params[4 ]; -struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER ,NULL }; -union acpi_object *out_obj; -u32 osc_dw0; -acpi_status *ret_status = (acpi_status *)retval; -struct acpi_osc_data *osc_data; -u32 flags = (unsigned long )context, temp; -acpi_handle tmp; - - status =acpi_get_handle (handle,"_OSC" , &tmp); -if (ACPI_FAILURE (status)) -return status; - - osc_data =acpi_get_osc_data (handle); -if (!osc_data) { -printk (KERN_ERR "acpi osc data array is full\n" ); -return AE_ERROR ; - } - - osc_data->ctrlset_buf[OSC_SUPPORT_TYPE ] |= (flags &OSC_SUPPORT_MASKS ); - -/* do _OSC query for all possible controls */ - temp = osc_data->ctrlset_buf[OSC_CONTROL_TYPE ]; - osc_data->ctrlset_buf[OSC_QUERY_TYPE ] =OSC_QUERY_ENABLE ; - osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] =OSC_CONTROL_MASKS ; +acpi_status status; +struct acpi_object_list input; +union acpi_object in_params[4 ]; +struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER ,NULL }; +union acpi_object *out_obj; +u32 osc_dw0, flags = osc_data->ctrlset_buf[OSC_QUERY_TYPE ];/* Setting up input parameters */ input.count =4 ;
@@ -97,14 +72,13 @@ acpi_query_osc ( status =acpi_evaluate_object (handle,"_OSC" , &input, &output);if (ACPI_FAILURE (status)) - goto out_nofree; - out_obj = output.pointer; +return status; + out_obj = output.pointer;if (out_obj->type !=ACPI_TYPE_BUFFER ) { -printk (KERN_DEBUG -"Evaluate _OSC returns wrong type\n" ); +printk (KERN_DEBUG "Evaluate _OSC returns wrong type\n" ); status =AE_TYPE ; - goto query_osc_out; + goto out_kfree; } osc_dw0 = *((u32 *) out_obj->buffer.pointer);if (osc_dw0) {
@@ -115,93 +89,64 @@ acpi_query_osc (if (osc_dw0 &OSC_INVALID_REVISION_ERROR )printk (KERN_DEBUG "_OSC invalid revision\n" );if (osc_dw0 &OSC_CAPABILITIES_MASK_ERROR ) { -/* Update Global Control Set */ - osc_data->global_ctrlsets = - *((u32 *)(out_obj->buffer.pointer +8 )); - status =AE_OK ; - goto query_osc_out; +if (flags &OSC_QUERY_ENABLE ) + goto out_success; +printk (KERN_DEBUG "_OSC FW not grant req. control\n" ); + status =AE_SUPPORT ; + goto out_kfree; } status =AE_ERROR ; - goto query_osc_out; + goto out_kfree; + } +out_success: +if (flags &OSC_QUERY_ENABLE ) { +/* Update Global Control Set */ + osc_data->global_ctrlsets = + *((u32 *)(out_obj->buffer.pointer +8 )); } - -/* Update Global Control Set */ - osc_data->global_ctrlsets = *((u32 *)(out_obj->buffer.pointer +8 )); status =AE_OK ; -query_osc_out: +out_kfree:kfree (output.pointer); -out_nofree: - *ret_status = status; - - osc_data->ctrlset_buf[OSC_QUERY_TYPE ] = !OSC_QUERY_ENABLE ; - osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] = temp; -if (ACPI_FAILURE (status)) { -/* no osc support at all */ - osc_data->ctrlset_buf[OSC_SUPPORT_TYPE ] =0 ; - } -return status; } - -static acpi_status -acpi_run_osc ( -acpi_handle handle, -void *context) +static acpi_status acpi_query_osc (acpi_handle handle, +u32 level,void *context,void **retval) { -acpi_status status; -struct acpi_object_list input; -union acpi_object in_params[4 ]; -struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER ,NULL }; -union acpi_object *out_obj; -u32 osc_dw0; - -/* Setting up input parameters */ - input.count =4 ; - input.pointer = in_params; - in_params[0 ].type =ACPI_TYPE_BUFFER ; - in_params[0 ].buffer.length =16 ; - in_params[0 ].buffer.pointer =OSC_UUID ; - in_params[1 ].type =ACPI_TYPE_INTEGER ; - in_params[1 ].integer.value =1 ; - in_params[2 ].type =ACPI_TYPE_INTEGER ; - in_params[2 ].integer.value =3 ; - in_params[3 ].type =ACPI_TYPE_BUFFER ; - in_params[3 ].buffer.length =12 ; - in_params[3 ].buffer.pointer = (u8 *)context; +acpi_status status; +acpi_status *ret_status = (acpi_status *)retval; +struct acpi_osc_data *osc_data; +u32 flags = (unsigned long )context, temp; +acpi_handle tmp; - status =acpi_evaluate_object (handle,"_OSC" , &input, &output); -if (ACPI_FAILURE (status)) + status =acpi_get_handle (handle,"_OSC" , &tmp); +if (ACPI_FAILURE (status))return status; - out_obj = output.pointer; -if (out_obj->type !=ACPI_TYPE_BUFFER ) { -printk (KERN_DEBUG -"Evaluate _OSC returns wrong type\n" ); - status =AE_TYPE ; - goto run_osc_out; + osc_data =acpi_get_osc_data (handle); +if (!osc_data) { +printk (KERN_ERR "acpi osc data array is full\n" ); +return AE_ERROR ; } - osc_dw0 = *((u32 *) out_obj->buffer.pointer); -if (osc_dw0) { -if (osc_dw0 &OSC_REQUEST_ERROR ) -printk (KERN_DEBUG "_OSC request fails\n" ); -if (osc_dw0 &OSC_INVALID_UUID_ERROR ) -printk (KERN_DEBUG "_OSC invalid UUID\n" ); -if (osc_dw0 &OSC_INVALID_REVISION_ERROR ) -printk (KERN_DEBUG "_OSC invalid revision\n" ); -if (osc_dw0 &OSC_CAPABILITIES_MASK_ERROR ) { -printk (KERN_DEBUG "_OSC FW not grant req. control\n" ); - status =AE_SUPPORT ; - goto run_osc_out; - } - status =AE_ERROR ; - goto run_osc_out; + + osc_data->ctrlset_buf[OSC_SUPPORT_TYPE ] |= (flags &OSC_SUPPORT_MASKS ); + +/* do _OSC query for all possible controls */ + temp = osc_data->ctrlset_buf[OSC_CONTROL_TYPE ]; + osc_data->ctrlset_buf[OSC_QUERY_TYPE ] =OSC_QUERY_ENABLE ; + osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] =OSC_CONTROL_MASKS ; + + status =acpi_run_osc (handle, osc_data); + *ret_status = status; + + osc_data->ctrlset_buf[OSC_QUERY_TYPE ] = !OSC_QUERY_ENABLE ; + osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] = temp; +if (ACPI_FAILURE (status)) { +/* no osc support at all */ + osc_data->ctrlset_buf[OSC_SUPPORT_TYPE ] =0 ; } - status =AE_OK ; -run_osc_out: -kfree (output.pointer);return status; }
@@ -260,7 +205,7 @@ acpi_status pci_osc_control_set(acpi_hanreturn AE_SUPPORT ; } osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] |= ctrlset; - status =acpi_run_osc (handle, osc_data->ctrlset_buf); + status =acpi_run_osc (handle, osc_data);if (ACPI_FAILURE (status)) { osc_data->ctrlset_buf[OSC_CONTROL_TYPE ] &= ~ctrlset; }
PATCH 3/5 pci-acpi: add flag to indicate query had been done
In reply to Kenji KaneshigeCurrent pci-acpi implementation checks osc_data->support_stat to see if control bits had been already queried. It is not good from the viewpoint of easy understanding. So this patch adds new 'is_queried' flag to indicate query had been done.
--- drivers/pci/pci-acpi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/pci/pci-acpi.c +3 -1
@@ -23,6 +23,7 @@ struct acpi_osc_data {acpi_handle handle;u32 support_set;u32 control_set; +int is_queried;u32 query_result;struct list_head sibiling; };
@@ -147,6 +148,7 @@ static acpi_status acpi_query_osc(acpi_hif (ACPI_SUCCESS (status)) { osc_data->support_set = support_set; osc_data->query_result = osc_args.query_result; + osc_data->is_queried =1 ; }return status;
@@ -203,7 +205,7 @@ acpi_status pci_osc_control_set(acpi_hanif (!ctrlset)return AE_TYPE ; -if (osc_data->support_set && +if (osc_data->is_queried && ((osc_data->query_result & ctrlset) != ctrlset))return AE_SUPPORT ;
PATCH 4/5 pci-acpi: remove unused variable in __pci_osc_support_set
In reply to Kenji KaneshigeThe 'retval' variable in __pci_osc_support_set() is no longer used. Remove this unused variable.
--- drivers/pci/pci-acpi.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
drivers/pci/pci-acpi.c +4 -11
@@ -120,7 +120,6 @@ static acpi_status acpi_query_osc(acpi_h u32 level, void *context, void **retval) { acpi_status status; -acpi_status *ret_status = (acpi_status *)retval;struct acpi_osc_data *osc_data;u32 flags = (unsigned long )context, support_set;acpi_handle tmp;
@@ -143,8 +142,6 @@ static acpi_status acpi_query_osc(acpi_h osc_args.capbuf[OSC_CONTROL_TYPE ] =OSC_CONTROL_MASKS ; status =acpi_run_osc (handle, &osc_args); - *ret_status = status; -if (ACPI_SUCCESS (status)) { osc_data->support_set = support_set; osc_data->query_result = osc_args.query_result;
@@ -164,15 +161,11 @@ static acpi_status acpi_query_osc(acpi_h**/ acpi_status __pci_osc_support_set (u32 flags,const char *hid) { -acpi_status retval =AE_NOT_FOUND ; - -if (!(flags &OSC_SUPPORT_MASKS )) { +if (!(flags &OSC_SUPPORT_MASKS ))return AE_TYPE ; - } -acpi_get_devices (hid, - acpi_query_osc, - (void *)(unsigned long )flags, - (void **) &retval ); + +acpi_get_devices (hid, acpi_query_osc, + (void *)(unsigned long )flags,NULL );return AE_OK ; }
PATCH 5/5 pci-acpi: minor cleanups for _OSC
In reply to Kenji KaneshigeMinor cleanup in pci-acpi.c.
--- drivers/pci/pci-acpi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/pci/pci-acpi.c +3 -2
@@ -51,7 +51,8 @@ static struct acpi_osc_data *acpi_get_osreturn data; } -static u8 OSC_UUID [16 ] = {0x5B ,0x4D ,0xDB ,0x33 ,0xF7 ,0x1F ,0x1C ,0x40 ,0x96 ,0x57 ,0x74 ,0x41 ,0xC0 ,0x3D ,0xD7 ,0x66 }; +static u8 OSC_UUID [16 ] = {0x5B ,0x4D ,0xDB ,0x33 ,0xF7 ,0x1F ,0x1C ,0x40 , +0x96 ,0x57 ,0x74 ,0x41 ,0xC0 ,0x3D ,0xD7 ,0x66 };static acpi_status acpi_run_osc (acpi_handle handle,struct acpi_osc_args *osc_args)
@@ -87,7 +88,7 @@ static acpi_status acpi_run_osc(acpi_han status =AE_TYPE ; goto out_kfree; } - osc_dw0 = *((u32 *) out_obj->buffer.pointer); + osc_dw0 = *((u32 *)out_obj->buffer.pointer);if (osc_dw0) {if (osc_dw0 &OSC_REQUEST_ERROR )printk (KERN_DEBUG "_OSC request fails\n" );
Quoted 11 lines · Kenji Kaneshige
Hi,
Here are some improvement patches for
_OSCevaluation on PCI.- [PATCH 1/5] pci-acpi: remove duplicate code for
_OSC
- [PATCH 2/5] pci-acpi: use local buffer for_OSC
- [PATCH 3/5] pci-acpi: add flag to indicate query had been done
- [PATCH 4/5] pci-acpi: remove unused variable in__pci_osc_support_set
- [PATCH 5/5] pci-acpi: minor cleanups for_OSCFor details, please see the header of each patch.
These cleanups look nice; Shaohua maybe you could take a look too and ack/nack?
Thanks,
Jesse
Quoted 11 lines · Jesse Barnes
Quoted 10 lines · Kenji Kaneshige
Hi,
Here are some improvement patches for
_OSCevaluation on PCI.- [PATCH 1/5] pci-acpi: remove duplicate code for
_OSC
- [PATCH 2/5] pci-acpi: use local buffer for_OSC
- [PATCH 3/5] pci-acpi: add flag to indicate query had been done
- [PATCH 4/5] pci-acpi: remove unused variable in__pci_osc_support_set
- [PATCH 5/5] pci-acpi: minor cleanups for_OSC
Quoted 4 lines
Quoted 1 line
For details, please see the header of each patch.
These cleanups look nice; Shaohua maybe you could take a look too and ack/nack?
Looks good. ACKed.
Thanks,
Shaohua
Quoted 12 lines · Shaohua Li
Quoted 11 lines · Jesse Barnes
Quoted 10 lines · Kenji Kaneshige
Hi,
Here are some improvement patches for
_OSCevaluation on PCI.- [PATCH 1/5] pci-acpi: remove duplicate code for
_OSC
- [PATCH 2/5] pci-acpi: use local buffer for_OSC
- [PATCH 3/5] pci-acpi: add flag to indicate query had been done
- [PATCH 4/5] pci-acpi: remove unused variable in__pci_osc_support_set
- [PATCH 5/5] pci-acpi: minor cleanups for_OSC
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
For details, please see the header of each patch.
These cleanups look nice; Shaohua maybe you could take a look too and ack/nack?
Looks good. ACKed.
Great, thanks for the review. Applied to linux-next.
Thanks,
Jesse