[PATCH] firmware: arm_sdei: remove unused interfaces

[PATCH] firmware: arm_sdei: remove unused interfaces

[Christoph Hellwig]

The export symbols to register/unregister and enable/disable events
aren't ever used outside of arm_sdei.c, so mark them static.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/firmware/arm_sdei.c | 13 +++++--------
 include/linux/arm_sdei.h    | 15 ---------------
 2 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 334c8be0c11fb..bdd6461647d74 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -400,7 +400,7 @@ static void _local_event_enable(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-int sdei_event_enable(u32 event_num)
+static int sdei_event_enable(u32 event_num)
 {
 	int err = -EINVAL;
 	struct sdei_event *event;
@@ -429,7 +429,6 @@ int sdei_event_enable(u32 event_num)
 
 	return err;
 }
-EXPORT_SYMBOL(sdei_event_enable);
 
 static int sdei_api_event_disable(u32 event_num)
 {
@@ -447,7 +446,7 @@ static void _ipi_event_disable(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-int sdei_event_disable(u32 event_num)
+static int sdei_event_disable(u32 event_num)
 {
 	int err = -EINVAL;
 	struct sdei_event *event;
@@ -471,7 +470,6 @@ int sdei_event_disable(u32 event_num)
 
 	return err;
 }
-EXPORT_SYMBOL(sdei_event_disable);
 
 static int sdei_api_event_unregister(u32 event_num)
 {
@@ -502,7 +500,7 @@ static int _sdei_event_unregister(struct sdei_event *event)
 	return sdei_do_cross_call(_local_event_unregister, event);
 }
 
-int sdei_event_unregister(u32 event_num)
+static int sdei_event_unregister(u32 event_num)
 {
 	int err;
 	struct sdei_event *event;
@@ -533,7 +531,6 @@ int sdei_event_unregister(u32 event_num)
 
 	return err;
 }
-EXPORT_SYMBOL(sdei_event_unregister);
 
 /*
  * unregister events, but don't destroy them as they are re-registered by
@@ -603,7 +600,8 @@ static int _sdei_event_register(struct sdei_event *event)
 	return err;
 }
 
-int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
+static int sdei_event_register(u32 event_num, sdei_event_callback *cb,
+		void *arg)
 {
 	int err;
 	struct sdei_event *event;
@@ -643,7 +641,6 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 
 	return err;
 }
-EXPORT_SYMBOL(sdei_event_register);
 
 static int sdei_reregister_event_llocked(struct sdei_event *event)
 {
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index 0a241c5c911d8..5f9fb1d95d515 100644
--- a/include/linux/arm_sdei.h
+++ b/include/linux/arm_sdei.h
@@ -22,21 +22,6 @@
  */
 typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
 
-/*
- * Register your callback to claim an event. The event must be described
- * by firmware.
- */
-int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg);
-
-/*
- * Calls to sdei_event_unregister() may return EINPROGRESS. Keep calling
- * it until it succeeds.
- */
-int sdei_event_unregister(u32 event_num);
-
-int sdei_event_enable(u32 event_num);
-int sdei_event_disable(u32 event_num);
-
 /* GHES register/unregister helpers */
 int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
 		       sdei_event_callback *critical_cb);
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_sdei: remove unused interfaces

[James Morse]

Hi Christoph,

(CC: +Xiongfeng)

Thanks for the reminder - I was just searching for who was using this.

On 04/05/2020 17:42, Christoph Hellwig wrote:
> The export symbols to register/unregister and enable/disable events
> aren't ever used outside of arm_sdei.c, so mark them static.

Xiongfeng, you have drivers using this, could they be posted upstream. Or can we stop
exporting these?


(they were originally added for the GHES RAS thing, but by the time it was all merged
upstream, it wasn't possible to build it as a module)

Thanks,

James


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 334c8be0c11fb..bdd6461647d74 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -400,7 +400,7 @@ static void _local_event_enable(void *data)
>  	sdei_cross_call_return(arg, err);
>  }
>  
> -int sdei_event_enable(u32 event_num)
> +static int sdei_event_enable(u32 event_num)
>  {
>  	int err = -EINVAL;
>  	struct sdei_event *event;
> @@ -429,7 +429,6 @@ int sdei_event_enable(u32 event_num)
>  
>  	return err;
>  }
> -EXPORT_SYMBOL(sdei_event_enable);
>  
>  static int sdei_api_event_disable(u32 event_num)
>  {
> @@ -447,7 +446,7 @@ static void _ipi_event_disable(void *data)
>  	sdei_cross_call_return(arg, err);
>  }
>  
> -int sdei_event_disable(u32 event_num)
> +static int sdei_event_disable(u32 event_num)
>  {
>  	int err = -EINVAL;
>  	struct sdei_event *event;
> @@ -471,7 +470,6 @@ int sdei_event_disable(u32 event_num)
>  
>  	return err;
>  }
> -EXPORT_SYMBOL(sdei_event_disable);
>  
>  static int sdei_api_event_unregister(u32 event_num)
>  {
> @@ -502,7 +500,7 @@ static int _sdei_event_unregister(struct sdei_event *event)
>  	return sdei_do_cross_call(_local_event_unregister, event);
>  }
>  
> -int sdei_event_unregister(u32 event_num)
> +static int sdei_event_unregister(u32 event_num)
>  {
>  	int err;
>  	struct sdei_event *event;
> @@ -533,7 +531,6 @@ int sdei_event_unregister(u32 event_num)
>  
>  	return err;
>  }
> -EXPORT_SYMBOL(sdei_event_unregister);
>  
>  /*
>   * unregister events, but don't destroy them as they are re-registered by
> @@ -603,7 +600,8 @@ static int _sdei_event_register(struct sdei_event *event)
>  	return err;
>  }
>  
> -int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
> +static int sdei_event_register(u32 event_num, sdei_event_callback *cb,
> +		void *arg)
>  {
>  	int err;
>  	struct sdei_event *event;
> @@ -643,7 +641,6 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  
>  	return err;
>  }
> -EXPORT_SYMBOL(sdei_event_register);
>  
>  static int sdei_reregister_event_llocked(struct sdei_event *event)
>  {
> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> index 0a241c5c911d8..5f9fb1d95d515 100644
> --- a/include/linux/arm_sdei.h
> +++ b/include/linux/arm_sdei.h
> @@ -22,21 +22,6 @@
>   */
>  typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>  
> -/*
> - * Register your callback to claim an event. The event must be described
> - * by firmware.
> - */
> -int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg);
> -
> -/*
> - * Calls to sdei_event_unregister() may return EINPROGRESS. Keep calling
> - * it until it succeeds.
> - */
> -int sdei_event_unregister(u32 event_num);
> -
> -int sdei_event_enable(u32 event_num);
> -int sdei_event_disable(u32 event_num);
> -
>  /* GHES register/unregister helpers */
>  int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
>  		       sdei_event_callback *critical_cb);
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_sdei: remove unused interfaces

[Xiongfeng Wang]

Hi James,

On 2020/5/5 1:14, James Morse wrote:
> Hi Christoph,
> 
> (CC: +Xiongfeng)
> 
> Thanks for the reminder - I was just searching for who was using this.

Thanks for CC me. We do have a driver that are using it.

> 
> On 04/05/2020 17:42, Christoph Hellwig wrote:
>> The export symbols to register/unregister and enable/disable events
>> aren't ever used outside of arm_sdei.c, so mark them static.
> 
> Xiongfeng, you have drivers using this, could they be posted upstream. Or can we stop
> exporting these?

It's the SDEI Wathchdog which is used for hardlockup detection. But I wasn't
able to push it upstream because we have Pseudo-NMI in mainline.

> 
> 
> (they were originally added for the GHES RAS thing, but by the time it was all merged
> upstream, it wasn't possible to build it as a module)

The SDEI Watchdog driver also can't be built as a module. We still need to
modify the origin kernel. So I think this patch doesn't affect me. Thanks for CC me.

Thanks,
Xiongfeng

> 
> Thanks,
> 
> James
> 
> 
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index 334c8be0c11fb..bdd6461647d74 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -400,7 +400,7 @@ static void _local_event_enable(void *data)
>>  	sdei_cross_call_return(arg, err);
>>  }
>>  
>> -int sdei_event_enable(u32 event_num)
>> +static int sdei_event_enable(u32 event_num)
>>  {
>>  	int err = -EINVAL;
>>  	struct sdei_event *event;
>> @@ -429,7 +429,6 @@ int sdei_event_enable(u32 event_num)
>>  
>>  	return err;
>>  }
>> -EXPORT_SYMBOL(sdei_event_enable);
>>  
>>  static int sdei_api_event_disable(u32 event_num)
>>  {
>> @@ -447,7 +446,7 @@ static void _ipi_event_disable(void *data)
>>  	sdei_cross_call_return(arg, err);
>>  }
>>  
>> -int sdei_event_disable(u32 event_num)
>> +static int sdei_event_disable(u32 event_num)
>>  {
>>  	int err = -EINVAL;
>>  	struct sdei_event *event;
>> @@ -471,7 +470,6 @@ int sdei_event_disable(u32 event_num)
>>  
>>  	return err;
>>  }
>> -EXPORT_SYMBOL(sdei_event_disable);
>>  
>>  static int sdei_api_event_unregister(u32 event_num)
>>  {
>> @@ -502,7 +500,7 @@ static int _sdei_event_unregister(struct sdei_event *event)
>>  	return sdei_do_cross_call(_local_event_unregister, event);
>>  }
>>  
>> -int sdei_event_unregister(u32 event_num)
>> +static int sdei_event_unregister(u32 event_num)
>>  {
>>  	int err;
>>  	struct sdei_event *event;
>> @@ -533,7 +531,6 @@ int sdei_event_unregister(u32 event_num)
>>  
>>  	return err;
>>  }
>> -EXPORT_SYMBOL(sdei_event_unregister);
>>  
>>  /*
>>   * unregister events, but don't destroy them as they are re-registered by
>> @@ -603,7 +600,8 @@ static int _sdei_event_register(struct sdei_event *event)
>>  	return err;
>>  }
>>  
>> -int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>> +static int sdei_event_register(u32 event_num, sdei_event_callback *cb,
>> +		void *arg)
>>  {
>>  	int err;
>>  	struct sdei_event *event;
>> @@ -643,7 +641,6 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>  
>>  	return err;
>>  }
>> -EXPORT_SYMBOL(sdei_event_register);
>>  
>>  static int sdei_reregister_event_llocked(struct sdei_event *event)
>>  {
>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
>> index 0a241c5c911d8..5f9fb1d95d515 100644
>> --- a/include/linux/arm_sdei.h
>> +++ b/include/linux/arm_sdei.h
>> @@ -22,21 +22,6 @@
>>   */
>>  typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>>  
>> -/*
>> - * Register your callback to claim an event. The event must be described
>> - * by firmware.
>> - */
>> -int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg);
>> -
>> -/*
>> - * Calls to sdei_event_unregister() may return EINPROGRESS. Keep calling
>> - * it until it succeeds.
>> - */
>> -int sdei_event_unregister(u32 event_num);
>> -
>> -int sdei_event_enable(u32 event_num);
>> -int sdei_event_disable(u32 event_num);
>> -
>>  /* GHES register/unregister helpers */
>>  int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
>>  		       sdei_event_callback *critical_cb);
>>
> 
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_sdei: remove unused interfaces

[James Morse]

Hi Xiongfeng,

On 05/05/2020 03:08, Xiongfeng Wang wrote:
> On 2020/5/5 1:14, James Morse wrote:
>> Hi Christoph,
>>
>> (CC: +Xiongfeng)
>>
>> Thanks for the reminder - I was just searching for who was using this.
> 
> Thanks for CC me. We do have a driver that are using it.

>> On 04/05/2020 17:42, Christoph Hellwig wrote:
>>> The export symbols to register/unregister and enable/disable events
>>> aren't ever used outside of arm_sdei.c, so mark them static.
>>
>> Xiongfeng, you have drivers using this, could they be posted upstream. Or can we stop
>> exporting these?
> 
> It's the SDEI Wathchdog which is used for hardlockup detection. But I wasn't
> able to push it upstream because we have Pseudo-NMI in mainline.

Hmm, that shouldn't be directly relevant, unless your SDEI watchdog is using the
bindable-irq thing?


If your firmware offers an event-id for the watchdog, please upstream the driver. Half of
the event-id space is reserved for vendor stuff like this.

If firmware needs to be told to re-configure the watchdog irq to make this work, then pNMI
is a much better fit. Having firmware and linux modifying the irqchip hardware is a
nightmare best avoided.


>> (they were originally added for the GHES RAS thing, but by the time it was all merged
>> upstream, it wasn't possible to build it as a module)
> 
> The SDEI Watchdog driver also can't be built as a module. We still need to
> modify the origin kernel. So I think this patch doesn't affect me. Thanks for CC me.

Okay, I'll pick this up to drop the module exports.

I'd prefer not to make all this static as these register/unregister calls are the
interface that is supposed to be used. If we are going to gut it, we should do it completely.


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_sdei: remove unused interfaces

[Xiongfeng Wang]

Hi James,

On 2020/5/19 23:47, James Morse wrote:
> Hi Xiongfeng,
> 
> On 05/05/2020 03:08, Xiongfeng Wang wrote:
>> On 2020/5/5 1:14, James Morse wrote:
>>> Hi Christoph,
>>>
>>> (CC: +Xiongfeng)
>>>
>>> Thanks for the reminder - I was just searching for who was using this.
>>
>> Thanks for CC me. We do have a driver that are using it.
> 
>>> On 04/05/2020 17:42, Christoph Hellwig wrote:
>>>> The export symbols to register/unregister and enable/disable events
>>>> aren't ever used outside of arm_sdei.c, so mark them static.
>>>
>>> Xiongfeng, you have drivers using this, could they be posted upstream. Or can we stop
>>> exporting these?
>>
>> It's the SDEI Wathchdog which is used for hardlockup detection. But I wasn't
>> able to push it upstream because we have Pseudo-NMI in mainline.
> 
> Hmm, that shouldn't be directly relevant, unless your SDEI watchdog is using the
> bindable-irq thing?

Yes, we are using the bindable-irq thing. It's easier for the UEFI team.
Firmware doesn't need to too much modification.

> 
> 
> If your firmware offers an event-id for the watchdog, please upstream the driver. Half of
> the event-id space is reserved for vendor stuff like this.

My origin thought is using a new event-id. But the firmware guys are new to SDEI
and don't know how to add a new event-id.

> 
> If firmware needs to be told to re-configure the watchdog irq to make this work, then pNMI
> is a much better fit. Having firmware and linux modifying the irqchip hardware is a
> nightmare best avoided.
> 

Yes, we are using the secure timer irq as the watchdog irq. So the firmware
needs to disable and enable the irq when we disable and enable the event. But
linux don't need to modify the irqchip hardware about the secure timer irq.

All works well except the kdump situation. Because in sdei_handler the secure
timer irq, which is routed to EL3, remains active. We clear the EOI when we
complete the event. So when we panic and start kdump in sdei_handler, the EL2
interrupt of the second kernel can not be taken. We add a hack to clear the EOI
before start kdump.

Yes, the pNMI is a much better fit. We don't need such a hack. But the product
department seems to have get used to the SDEI watchdog method. It works well and
they don't want much change.

Thanks,
Xiongfeng

> 
>>> (they were originally added for the GHES RAS thing, but by the time it was all merged
>>> upstream, it wasn't possible to build it as a module)
>>
>> The SDEI Watchdog driver also can't be built as a module. We still need to
>> modify the origin kernel. So I think this patch doesn't affect me. Thanks for CC me.
> 
> Okay, I'll pick this up to drop the module exports.
> 
> I'd prefer not to make all this static as these register/unregister calls are the
> interface that is supposed to be used. If we are going to gut it, we should do it completely.
> 
> 
> Thanks,
> 
> James
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel