Re: [PATCH v3] dell_rbu: make firmware payload memory uncachable
On 4/4/2018 3:30 PM, Takashi Iwai wrote: > On Wed, 28 Mar 2018 17:07:47 +0200, > Stuart Hayes wrote: >> >> @@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length) >> invalid_addr_packet_array[idx++] = packet_data_temp_buf; >> packet_data_temp_buf = NULL; >> } >> +/* >> + * set to uncachable or it may never get written back before >> + * reboot >> + */ >> +set_memory_uc((unsigned long)packet_data_temp_buf, >> +1 << ordernum); > > Won't this cause Oops when the if-condition above meets? > Namely packet_data_temp_buf is set to NULL there. > > Maybe better to try a fault injection to check the error handling. > > > thanks, > > Takashi > Yes, thank you for catching my mistake. Stuart
Re: [PATCH v3] dell_rbu: make firmware payload memory uncachable
On 4/4/2018 3:30 PM, Takashi Iwai wrote: > On Wed, 28 Mar 2018 17:07:47 +0200, > Stuart Hayes wrote: >> >> @@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length) >> invalid_addr_packet_array[idx++] = packet_data_temp_buf; >> packet_data_temp_buf = NULL; >> } >> +/* >> + * set to uncachable or it may never get written back before >> + * reboot >> + */ >> +set_memory_uc((unsigned long)packet_data_temp_buf, >> +1 << ordernum); > > Won't this cause Oops when the if-condition above meets? > Namely packet_data_temp_buf is set to NULL there. > > Maybe better to try a fault injection to check the error handling. > > > thanks, > > Takashi > Yes, thank you for catching my mistake. Stuart
Re: [PATCH v3] dell_rbu: make firmware payload memory uncachable
On Wed, 28 Mar 2018 17:07:47 +0200, Stuart Hayes wrote: > > @@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length) > invalid_addr_packet_array[idx++] = packet_data_temp_buf; > packet_data_temp_buf = NULL; > } > + /* > + * set to uncachable or it may never get written back before > + * reboot > + */ > + set_memory_uc((unsigned long)packet_data_temp_buf, > + 1 << ordernum); Won't this cause Oops when the if-condition above meets? Namely packet_data_temp_buf is set to NULL there. Maybe better to try a fault injection to check the error handling. thanks, Takashi
Re: [PATCH v3] dell_rbu: make firmware payload memory uncachable
On Wed, 28 Mar 2018 17:07:47 +0200, Stuart Hayes wrote: > > @@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length) > invalid_addr_packet_array[idx++] = packet_data_temp_buf; > packet_data_temp_buf = NULL; > } > + /* > + * set to uncachable or it may never get written back before > + * reboot > + */ > + set_memory_uc((unsigned long)packet_data_temp_buf, > + 1 << ordernum); Won't this cause Oops when the if-condition above meets? Namely packet_data_temp_buf is set to NULL there. Maybe better to try a fault injection to check the error handling. thanks, Takashi
[PATCH v3] dell_rbu: make firmware payload memory uncachable
The dell_rbu driver takes firmware update payloads and puts them in memory so the system BIOS can find them after a reboot. This sometimes fails (though rarely), because the memory containing the payload is in the CPU cache but never gets written back to main memory before the system is rebooted (CPU cache contents are lost on reboot). With this patch, the payload memory will be changed to uncachable to ensure that the payload is actually in main memory before the system is rebooted. Signed-off-by: Stuart Hayes--- v2 Added include, removed extra parentheses v3 Corrected formatting and include line This driver has no maintainer. diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c index 2f452f1f7c8a..778efbb72c76 100644 --- a/drivers/firmware/dell_rbu.c +++ b/drivers/firmware/dell_rbu.c @@ -45,6 +45,7 @@ #include #include #include +#include MODULE_AUTHOR("Abhay Salunke "); MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems"); @@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length) invalid_addr_packet_array[idx++] = packet_data_temp_buf; packet_data_temp_buf = NULL; } + /* +* set to uncachable or it may never get written back before +* reboot +*/ + set_memory_uc((unsigned long)packet_data_temp_buf, + 1 << ordernum); } spin_lock(_data.lock); @@ -349,6 +356,8 @@ static void packet_empty_list(void) * to make sure there are no stale RBU packets left in memory */ memset(newpacket->data, 0, rbu_data.packetsize); + set_memory_wb((unsigned long)newpacket->data, + 1 << newpacket->ordernum); free_pages((unsigned long) newpacket->data, newpacket->ordernum); kfree(newpacket);
[PATCH v3] dell_rbu: make firmware payload memory uncachable
The dell_rbu driver takes firmware update payloads and puts them in memory so the system BIOS can find them after a reboot. This sometimes fails (though rarely), because the memory containing the payload is in the CPU cache but never gets written back to main memory before the system is rebooted (CPU cache contents are lost on reboot). With this patch, the payload memory will be changed to uncachable to ensure that the payload is actually in main memory before the system is rebooted. Signed-off-by: Stuart Hayes --- v2 Added include, removed extra parentheses v3 Corrected formatting and include line This driver has no maintainer. diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c index 2f452f1f7c8a..778efbb72c76 100644 --- a/drivers/firmware/dell_rbu.c +++ b/drivers/firmware/dell_rbu.c @@ -45,6 +45,7 @@ #include #include #include +#include MODULE_AUTHOR("Abhay Salunke "); MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems"); @@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length) invalid_addr_packet_array[idx++] = packet_data_temp_buf; packet_data_temp_buf = NULL; } + /* +* set to uncachable or it may never get written back before +* reboot +*/ + set_memory_uc((unsigned long)packet_data_temp_buf, + 1 << ordernum); } spin_lock(_data.lock); @@ -349,6 +356,8 @@ static void packet_empty_list(void) * to make sure there are no stale RBU packets left in memory */ memset(newpacket->data, 0, rbu_data.packetsize); + set_memory_wb((unsigned long)newpacket->data, + 1 << newpacket->ordernum); free_pages((unsigned long) newpacket->data, newpacket->ordernum); kfree(newpacket);