Re: mmc: sdhci-pci: why no .shutdown() implemented

[Adrian Hunter <adrian.hunter@intel.com>]

On 12/03/12 16:30, Mansoor, Illyas wrote:
>>> On our platform we implemented a BUG in pci_set_power_state callback
>>> when the shutdown Is in progress, and we caught sdhci-pci doing
>>> pci_set_power_state(D0) when shutdown was Already in progress.
>>>
>>> My question, why doesn't sdhci-pci.c implement a .shutdown() callback
>>> and close the device After doing sys_sync()?
>>>
>>> Is there some reason behind it not doing a graceful shutdown.
>>
>> In general, the kernel does not know how to do a graceful shutdown.  There may
>> be any number of housekeeping activities that userspace wishes to do before
>> shutting down.
> 
> I see some storage drivers do implement .shutdown() callbacks do you think
> sdhci-pci
> could also do the same?

Certainly.

> 
> immansoo@immansoo-desktop:~/Linux_kernel/linux-stable/drivers/mmc$ find . -name
> \*.c -exec grep \.shutdown -H {} \;
> ./host/s3cmci.c:static void s3cmci_shutdown(struct platform_device *pdev)
> ./host/s3cmci.c:        s3cmci_shutdown(pdev);
> ./host/s3cmci.c:        .shutdown       = s3cmci_shutdown,
> 
>>
>> If file systems are not getting sync'd that is a userspace problem, not a
> kernel
>> problem.
> 
> I agree the issue needs to be fixed in user-space, so it does not request
> Storage device during shutdown. 
> 
> Shouldn't we also make sure we don't open a device during shutdown, that way we
> make
> sure at least we don't stay in device power'd up state during power-off.
> 
> It's not that the file system is not getting sync'd, but issue is bringing  up
> the device
> during shutdown, because once the shutdown starts there is no way to stop it
> and the device may be left in undefined state.

Power-off is not an undefined state.

What exactly is the problem?

> 
> 
>>
>>>
>>> I think it could cause corruption, since there is no guarantee when
>>> the system will power-off/reboot/halt
>>>
>>> I just implemented a patch to fix this, if it's a good fix I'll submit.
>>>
>>> diff --git a/drivers/mmc/host/sdhci-pci.c
>>> b/drivers/mmc/host/sdhci-pci.c index f5fe05c..e0818c9 100644
>>> --- a/drivers/mmc/host/sdhci-pci.c
>>> +++ b/drivers/mmc/host/sdhci-pci.c
>>> @@ -23,6 +23,7 @@
>>>  #include <linux/regulator/consumer.h>  #include <linux/pm_runtime.h>
>>> #include <linux/async.h>
>>> +#include <linux/syscalls.h>
>>>
>>>  #include <asm/scatterlist.h>
>>>  #include <asm/io.h>
>>> @@ -1342,6 +1343,34 @@ err:
>>>         return ret;
>>>  }
>>>
>>> +static void sdhci_pci_shutdown(struct pci_dev *pdev) {
>>> +       int i;
>>> +       struct sdhci_pci_chip *chip;
>>> +
>>> +       printk(KERN_INFO "%s: Syncing filesystems ... ", __func__);
>>> +       sys_sync();
>>> +       printk("done.\n");
>>> +
>>> +       pm_runtime_get_sync(&pdev->dev);
>>> +
>>> +       chip = pci_get_drvdata(pdev);
>>> +
>>> +       if (chip) {
>>> +               for (i = 0;i < chip->num_slots; i++)
>>> +                       sdhci_pci_remove_slot(chip->slots[i]);
>>> +
>>> +               pci_set_drvdata(pdev, NULL);
>>> +               kfree(chip);
>>> +       }
>>> +
>>> +       pci_disable_device(pdev);
>>> +
>>> +       pm_runtime_put_noidle(&pdev->dev);
>>> +       pm_runtime_forbid(&pdev->dev);
>>> +       pm_runtime_disable(&pdev->dev); }
>>> +
>>>  static void __devexit sdhci_pci_remove(struct pci_dev *pdev)  {
>>>         int i;
>>> @@ -1473,6 +1502,7 @@ static struct pci_driver sdhci_driver = {
>>>         .id_table =     pci_ids,
>>>         .probe =        sdhci_pci_probe,
>>>         .remove =       __devexit_p(sdhci_pci_remove),
>>> +       .shutdown =     sdhci_pci_shutdown,
>>>
>>>
>>> -Illyas
>