[BUG] ata: pata_marvell: Warning when probing the module

[BUG] ata: pata_marvell: Warning when probing the module

[Zheyu Ma]

Hello,

I found a bug in the pata_marvell module.
When probing the driver, it seems to trigger the error path and
executes the function marvell_cable_detect(), but the
'ap->ioaddr.bmdma_addr' is not initialized, which causes a warning.

The following log can reveal it:

[    3.453943] Bad IO access at port 0x1 (return inb(port))
[    3.454430] WARNING: CPU: 7 PID: 291 at lib/iomap.c:44 ioread8+0x4a/0x60
[    3.457962] RIP: 0010:ioread8+0x4a/0x60
[    3.466362] Call Trace:
[    3.466572]  <TASK>
[    3.466756]  marvell_cable_detect+0xad/0xf0 [pata_marvell]
[    3.467699]  ata_eh_recover+0x3520/0x6cc0
[    3.473262]  ata_do_eh+0x49/0x3c0
[    3.473906]  ata_scsi_port_error_handler+0xd96/0x1d00
[    3.474355]  ata_scsi_error+0x243/0x290
[    3.475428]  scsi_error_handler+0x2ff/0xea0
[    3.477244]  kthread+0x262/0x2e0

Re: [BUG] ata: pata_marvell: Warning when probing the module

[Damien Le Moal]

On 4/10/22 15:30, Zheyu Ma wrote:
> Hello,
> 
> I found a bug in the pata_marvell module.
> When probing the driver, it seems to trigger the error path and
> executes the function marvell_cable_detect(), but the
> 'ap->ioaddr.bmdma_addr' is not initialized, which causes a warning.

I do not have this hardware so I cannot debug this. Please debug it and
send a patch. bmdma_addr is normally set in ata_pci_bmdma_init(), but some
drivers set it manually in their probe functions. No idea about the
marvell driver, I have not checked it.

> 
> The following log can reveal it:
> 
> [    3.453943] Bad IO access at port 0x1 (return inb(port))
> [    3.454430] WARNING: CPU: 7 PID: 291 at lib/iomap.c:44 ioread8+0x4a/0x60
> [    3.457962] RIP: 0010:ioread8+0x4a/0x60
> [    3.466362] Call Trace:
> [    3.466572]  <TASK>
> [    3.466756]  marvell_cable_detect+0xad/0xf0 [pata_marvell]
> [    3.467699]  ata_eh_recover+0x3520/0x6cc0
> [    3.473262]  ata_do_eh+0x49/0x3c0
> [    3.473906]  ata_scsi_port_error_handler+0xd96/0x1d00
> [    3.474355]  ata_scsi_error+0x243/0x290
> [    3.475428]  scsi_error_handler+0x2ff/0xea0
> [    3.477244]  kthread+0x262/0x2e0


-- 
Damien Le Moal
Western Digital Research

Re: [BUG] ata: pata_marvell: Warning when probing the module

[Zheyu Ma]

On Mon, Apr 11, 2022 at 8:09 AM Ozgur <ozgur@linux.com> wrote:
>
>
>
> 11.04.2022, 02:53, "Damien Le Moal" <damien.lemoal@opensource.wdc.com>:
>
> On 4/10/22 15:30, Zheyu Ma wrote:
>
>  Hello,
>
>  I found a bug in the pata_marvell module.
>  When probing the driver, it seems to trigger the error path and
>  executes the function marvell_cable_detect(), but the
>  'ap->ioaddr.bmdma_addr' is not initialized, which causes a warning.
>
>
>
> Hello,
> i'm not sure if this is a bug because you get as ap points to a port number.
>
> (ap->port_no)
>
> it points to 0x1 port that appears in error message.

Please correct me if i'm wrong, actually 'ap->port_no' is zero, and
the 'ap->ioaddr.bmdma_addr' is zero too since it is not initialized.

> otherwise BUG will work and if it cannot read warning will return.
> ( BUG(); is macro )

Zheyu Ma

Re: [BUG] ata: pata_marvell: Warning when probing the module

[Zheyu Ma]

On Mon, Apr 11, 2022 at 7:53 AM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 4/10/22 15:30, Zheyu Ma wrote:
> > Hello,
> >
> > I found a bug in the pata_marvell module.
> > When probing the driver, it seems to trigger the error path and
> > executes the function marvell_cable_detect(), but the
> > 'ap->ioaddr.bmdma_addr' is not initialized, which causes a warning.
>
> I do not have this hardware so I cannot debug this. Please debug it and
> send a patch. bmdma_addr is normally set in ata_pci_bmdma_init(), but some
> drivers set it manually in their probe functions. No idea about the
> marvell driver, I have not checked it.

To be honest I don't have a good solution to this problem, because
other drivers don't have similar behavior. The marvell driver doesn't
even initialize 'bmdma_addr' before calling 'cable_detect'.

So a simple idea I have is to check if 'bmdma_addr' is 0 before
reading it and if so return the error code ATA_CBL_NONE.

Zheyu Ma

Re: [BUG] ata: pata_marvell: Warning when probing the module

[Damien Le Moal]

On 4/12/22 15:34, Zheyu Ma wrote:
> On Mon, Apr 11, 2022 at 7:53 AM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
>>
>> On 4/10/22 15:30, Zheyu Ma wrote:
>>> Hello,
>>>
>>> I found a bug in the pata_marvell module.
>>> When probing the driver, it seems to trigger the error path and
>>> executes the function marvell_cable_detect(), but the
>>> 'ap->ioaddr.bmdma_addr' is not initialized, which causes a warning.
>>
>> I do not have this hardware so I cannot debug this. Please debug it and
>> send a patch. bmdma_addr is normally set in ata_pci_bmdma_init(), but some
>> drivers set it manually in their probe functions. No idea about the
>> marvell driver, I have not checked it.
> 
> To be honest I don't have a good solution to this problem, because
> other drivers don't have similar behavior. The marvell driver doesn't
> even initialize 'bmdma_addr' before calling 'cable_detect'.

Then this is the bug that needs to be fixed, no ?

> So a simple idea I have is to check if 'bmdma_addr' is 0 before
> reading it and if so return the error code ATA_CBL_NONE.

And if indeed, even after it is initialized it is still 0, then yes, this
change seems sensible.

> 
> Zheyu Ma


-- 
Damien Le Moal
Western Digital Research

Re: [BUG] ata: pata_marvell: Warning when probing the module

[Zheyu Ma]

On Wed, Apr 13, 2022 at 11:42 AM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 4/12/22 15:34, Zheyu Ma wrote:
> > On Mon, Apr 11, 2022 at 7:53 AM Damien Le Moal
> > <damien.lemoal@opensource.wdc.com> wrote:
> >>
> >> On 4/10/22 15:30, Zheyu Ma wrote:
> >>> Hello,
> >>>
> >>> I found a bug in the pata_marvell module.
> >>> When probing the driver, it seems to trigger the error path and
> >>> executes the function marvell_cable_detect(), but the
> >>> 'ap->ioaddr.bmdma_addr' is not initialized, which causes a warning.
> >>
> >> I do not have this hardware so I cannot debug this. Please debug it and
> >> send a patch. bmdma_addr is normally set in ata_pci_bmdma_init(), but some
> >> drivers set it manually in their probe functions. No idea about the
> >> marvell driver, I have not checked it.
> >
> > To be honest I don't have a good solution to this problem, because
> > other drivers don't have similar behavior. The marvell driver doesn't
> > even initialize 'bmdma_addr' before calling 'cable_detect'.
>
> Then this is the bug that needs to be fixed, no ?
>
> > So a simple idea I have is to check if 'bmdma_addr' is 0 before
> > reading it and if so return the error code ATA_CBL_NONE.
>
> And if indeed, even after it is initialized it is still 0, then yes, this
> change seems sensible.

Sorry for the late reply, I found the root cause of this issue.
The marvell driver execute the ata_pci_bmdma_init() function, but the
driver just returned at the following code snippet.

if (pci_resource_start(pdev, 4) == 0) {
      ata_bmdma_nodma(host, "BAR4 is zero");
      return;
}

So the driver didn't initialize the 'bmdma_addr' but used it in the
cable_detect() function.
It seems that the problem is caused by the hardware, is this a bug
that we should fix?

Zheyu Ma


Zheyu Ma

Re: [BUG] ata: pata_marvell: Warning when probing the module

[Damien Le Moal]

On 4/20/22 11:21, Zheyu Ma wrote:
> On Wed, Apr 13, 2022 at 11:42 AM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
>>
>> On 4/12/22 15:34, Zheyu Ma wrote:
>>> On Mon, Apr 11, 2022 at 7:53 AM Damien Le Moal
>>> <damien.lemoal@opensource.wdc.com> wrote:
>>>>
>>>> On 4/10/22 15:30, Zheyu Ma wrote:
>>>>> Hello,
>>>>>
>>>>> I found a bug in the pata_marvell module.
>>>>> When probing the driver, it seems to trigger the error path and
>>>>> executes the function marvell_cable_detect(), but the
>>>>> 'ap->ioaddr.bmdma_addr' is not initialized, which causes a warning.
>>>>
>>>> I do not have this hardware so I cannot debug this. Please debug it and
>>>> send a patch. bmdma_addr is normally set in ata_pci_bmdma_init(), but some
>>>> drivers set it manually in their probe functions. No idea about the
>>>> marvell driver, I have not checked it.
>>>
>>> To be honest I don't have a good solution to this problem, because
>>> other drivers don't have similar behavior. The marvell driver doesn't
>>> even initialize 'bmdma_addr' before calling 'cable_detect'.
>>
>> Then this is the bug that needs to be fixed, no ?
>>
>>> So a simple idea I have is to check if 'bmdma_addr' is 0 before
>>> reading it and if so return the error code ATA_CBL_NONE.
>>
>> And if indeed, even after it is initialized it is still 0, then yes, this
>> change seems sensible.
> 
> Sorry for the late reply, I found the root cause of this issue.
> The marvell driver execute the ata_pci_bmdma_init() function, but the
> driver just returned at the following code snippet.
> 
> if (pci_resource_start(pdev, 4) == 0) {
>       ata_bmdma_nodma(host, "BAR4 is zero");
>       return;
> }
> 
> So the driver didn't initialize the 'bmdma_addr' but used it in the
> cable_detect() function.
> It seems that the problem is caused by the hardware, is this a bug
> that we should fix?

So it looks like your adapter is saying: I do not support DMA.
In that case, having bmdma_addr as 0 should be expected and
pata_marvel_cable_detect() should check the address before attempting an
ioread8(). It is weird that the cable information is in that bar though...

In any case, you should check the adapter specs to verify how the cable
type can be detected. And if unknown when bmdma_addr is 0, then just
return ATA_CBL_PATA_UNK.

> 
> Zheyu Ma
> 
> 
> Zheyu Ma


-- 
Damien Le Moal
Western Digital Research

Re: [BUG] ata: pata_marvell: Warning when probing the module

[Zheyu Ma]

On Wed, Apr 20, 2022 at 11:07 AM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 4/20/22 11:21, Zheyu Ma wrote:
> > On Wed, Apr 13, 2022 at 11:42 AM Damien Le Moal
> > <damien.lemoal@opensource.wdc.com> wrote:
> >>
> >> On 4/12/22 15:34, Zheyu Ma wrote:
> >>> On Mon, Apr 11, 2022 at 7:53 AM Damien Le Moal
> >>> <damien.lemoal@opensource.wdc.com> wrote:
> >>>>
> >>>> On 4/10/22 15:30, Zheyu Ma wrote:
> >>>>> Hello,
> >>>>>
> >>>>> I found a bug in the pata_marvell module.
> >>>>> When probing the driver, it seems to trigger the error path and
> >>>>> executes the function marvell_cable_detect(), but the
> >>>>> 'ap->ioaddr.bmdma_addr' is not initialized, which causes a warning.
> >>>>
> >>>> I do not have this hardware so I cannot debug this. Please debug it and
> >>>> send a patch. bmdma_addr is normally set in ata_pci_bmdma_init(), but some
> >>>> drivers set it manually in their probe functions. No idea about the
> >>>> marvell driver, I have not checked it.
> >>>
> >>> To be honest I don't have a good solution to this problem, because
> >>> other drivers don't have similar behavior. The marvell driver doesn't
> >>> even initialize 'bmdma_addr' before calling 'cable_detect'.
> >>
> >> Then this is the bug that needs to be fixed, no ?
> >>
> >>> So a simple idea I have is to check if 'bmdma_addr' is 0 before
> >>> reading it and if so return the error code ATA_CBL_NONE.
> >>
> >> And if indeed, even after it is initialized it is still 0, then yes, this
> >> change seems sensible.
> >
> > Sorry for the late reply, I found the root cause of this issue.
> > The marvell driver execute the ata_pci_bmdma_init() function, but the
> > driver just returned at the following code snippet.
> >
> > if (pci_resource_start(pdev, 4) == 0) {
> >       ata_bmdma_nodma(host, "BAR4 is zero");
> >       return;
> > }
> >
> > So the driver didn't initialize the 'bmdma_addr' but used it in the
> > cable_detect() function.
> > It seems that the problem is caused by the hardware, is this a bug
> > that we should fix?
>
> So it looks like your adapter is saying: I do not support DMA.
> In that case, having bmdma_addr as 0 should be expected and
> pata_marvel_cable_detect() should check the address before attempting an
> ioread8(). It is weird that the cable information is in that bar though...
>
> In any case, you should check the adapter specs to verify how the cable
> type can be detected. And if unknown when bmdma_addr is 0, then just
> return ATA_CBL_PATA_UNK.

Thank you very much for your detailed explanation, it helped me a lot :)

Zheyu Ma