Am Montag, den 19.12.2016, 23:41 +0100 schrieb Martin Kaiser:
> The DryIce chipset has a dedicated security violation interrupt that is
> triggered for security violations (if configured to do so). According
> to the publicly available imx258 reference manual, irq 56 is used for
> this interrupt.
>
> Install a handler for the security violation interrupt if an irq for
> this is provided by platform data / device tree. Move the code for
> handling security violations from the "normal" interrupt handler into
> the security violation interrupt handler.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> v2:
> - make sec_irq optional to avoid breaking the Device Tree ABI
> - removed the Device Tree bindings, I'll prepare a separate patch for them
Please submit the binding as a separate patch in the same series.
Otherwise you are building de-facto DT bindings by changing the driver.
>
> drivers/rtc/rtc-imxdi.c | 68
> ++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
> index 67b56b8..ec6077a0 100644
> --- a/drivers/rtc/rtc-imxdi.c
> +++ b/drivers/rtc/rtc-imxdi.c
> @@ -109,6 +109,7 @@
> * @rtc: pointer to rtc struct
> * @ioaddr: IO registers pointer
> * @irq: dryice normal interrupt
> + * @sec_irq: dryice security violation interrupt
This isn't used outside the probe routine, so doesn't need to be saved
in the driver data.
> * @clk: input reference clock
> * @dsr: copy of the DSR register
> * @irq_lock: interrupt enable register (DIER) lock
> @@ -121,6 +122,7 @@ struct imxdi_dev {
> struct rtc_device *rtc;
> void __iomem *ioaddr;
> int irq;
> + int sec_irq;
> struct clk *clk;
> u32 dsr;
> spinlock_t irq_lock;
> @@ -688,24 +690,6 @@ static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
> dier = readl(imxdi->ioaddr + DIER);
> dsr = readl(imxdi->ioaddr + DSR);
>
> - /* handle the security violation event */
> - if (dier & DIER_SVIE) {
> - if (dsr & DSR_SVF) {
> - /*
> - * Disable the interrupt when this kind of event has
> - * happened.
> - * There cannot be more than one event of this type,
> - * because it needs a complex state change
> - * including a main power cycle to get again out of
> - * this state.
> - */
> - di_int_disable(imxdi, DIER_SVIE);
> - /* report the violation */
> - di_report_tamper_info(imxdi, dsr);
> - rc = IRQ_HANDLED;
> - }
> - }
> -
> /* handle write complete and write error cases */
> if (dier & DIER_WCIE) {
> /*If the write wait queue is empty then there is no pending
> @@ -743,6 +727,40 @@ static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
> }
>
> /*
> + * dryice security violation interrupt handler
> + */
> +static irqreturn_t dryice_sec_irq(int irq, void *dev_id)
> +{
> + struct imxdi_dev *imxdi = dev_id;
> + u32 dsr, dier;
> + irqreturn_t rc = IRQ_NONE;
> +
> + dier = readl(imxdi->ioaddr + DIER);
> + dsr = readl(imxdi->ioaddr + DSR);
> +
> + /* handle the security violation event */
> + if (dier & DIER_SVIE) {
> + if (dsr & DSR_SVF) {
> + /*
> + * Disable the interrupt when this kind of event has
> + * happened.
> + * There cannot be more than one event of this type,
> + * because it needs a complex state change
> + * including a main power cycle to get again out of
> + * this state.
> + */
> + di_int_disable(imxdi, DIER_SVIE);
> + /* report the violation */
> + di_report_tamper_info(imxdi, dsr);
> + rc = IRQ_HANDLED;
> + }
> + }
> +
> + return rc;
> +}
This separate handler isn't needed. It is reading the same IRQ status
register, so you can just leave the code as is and request the sec-irq
with the same "dryice_norm_irq" handler.
> +
> +
> +/*
> * post the alarm event from user context so it can sleep
> * on the write completion.
> */
> @@ -783,6 +801,13 @@ static int __init dryice_rtc_probe(struct
> platform_device *pdev)
> imxdi->irq = platform_get_irq(pdev, 0);
> if (imxdi->irq < 0)
> return imxdi->irq;
Insert blank line for better legibility.
> + /* the 2nd irq is the security violation irq
> + make this optional, don't break the device tree ABI */
> + rc = platform_get_irq(pdev, 1);
> + if (rc > 0)
> + imxdi->sec_irq = rc;
> + else
> + imxdi->sec_irq = IRQ_NOTCONNECTED;
>
> init_waitqueue_head(&imxdi->write_wait);
>
> @@ -815,6 +840,13 @@ static int __init dryice_rtc_probe(struct
> platform_device *pdev)
> goto err;
> }
>
> + rc = devm_request_irq(&pdev->dev, imxdi->sec_irq, dryice_sec_irq,
> + IRQF_SHARED, pdev->name, imxdi);
> + if (rc) {
> + dev_warn(&pdev->dev, "security violation interrupt not
> available.\n");
> + /* this is not an error, see above */
> + }
> +
Please just fold this into the "if (rc > 0)" path above.
> platform_set_drvdata(pdev, imxdi);
> imxdi->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> &dryice_rtc_ops, THIS_MODULE);
Regards,
Lucas
