Re: [PATCH RESEND] x86:pci: Change sta2x11_dma_ops stucture to use switolb_dma_supported as it's dma_supported function in sta2x11-fixup.c

Re: [PATCH RESEND] x86:pci: Change sta2x11_dma_ops stucture to use switolb_dma_supported as it's dma_supported function in sta2x11-fixup.c

[Ingo Molnar]

* Nicholas Krause <xerofoify@gmail.com> wrote:

> This changes the structure sta2x11_dma_ops stucture to 
> use switolb_dma_supported as it's function for 
> dma_supported hardware verus setting this value to NULL 
> as this should be set correctly for when dma_supported 
> function needs to be called for this hardware. Otherwise 
> this will cause a bug that will crash a operation needing 
> to access this function if an intended hardware operation 
> needs to call it but the kernel has the function pointer 
> for this structure set to NULL incorrectly.

This is a pretty vague description - what specific hardware 
is affected and how is the bug triggered?

Thanks,

	Ingo

Re: [PATCH RESEND] x86:pci: Change sta2x11_dma_ops stucture to use switolb_dma_supported as it's dma_supported function in sta2x11-fixup.c

[Bjorn Helgaas]

On Sun, Feb 15, 2015 at 1:54 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
> This changes the structure sta2x11_dma_ops stucture to use switolb_dma_supported
> as it's function for dma_supported hardware verus setting this value to NULL as
> this should be set correctly for when dma_supported function needs to be called
> for this hardware. Otherwise this will cause a bug that will crash a operation
> needing to access this function if an intended hardware operation needs to call
> it but the kernel has the function pointer for this structure set to NULL
> incorrectly.
>
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  arch/x86/pci/sta2x11-fixup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/pci/sta2x11-fixup.c b/arch/x86/pci/sta2x11-fixup.c
> index 5ceda85..d773a73 100644
> --- a/arch/x86/pci/sta2x11-fixup.c
> +++ b/arch/x86/pci/sta2x11-fixup.c
> @@ -191,7 +191,7 @@ static struct dma_map_ops sta2x11_dma_ops = {
>         .sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
>         .sync_sg_for_device = swiotlb_sync_sg_for_device,
>         .mapping_error = swiotlb_dma_mapping_error,
> -       .dma_supported = NULL, /* FIXME: we should use this instead! */
> +       .dma_supported = switolb_dma_supported,

I don't think this compiles.

>  };
>
>  /* At setup time, we use our own ops if the device is a ConneXt one */
> --
> 2.1.0
>

Re: [PATCH RESEND] x86:pci: Change sta2x11_dma_ops stucture to use switolb_dma_supported as it's dma_supported function in sta2x11-fixup.c

[Ingo Molnar]

* nick <xerofoify@gmail.com> wrote:

> 
> 
> On 2015-02-16 02:55 AM, Ingo Molnar wrote:
> > 
> > * Nicholas Krause <xerofoify@gmail.com> wrote:
> > 
> >> This changes the structure sta2x11_dma_ops stucture to 
> >> use switolb_dma_supported as it's function for 
> >> dma_supported hardware verus setting this value to NULL 
> >> as this should be set correctly for when dma_supported 
> >> function needs to be called for this hardware. Otherwise 
> >> this will cause a bug that will crash a operation needing 
> >> to access this function if an intended hardware operation 
> >> needs to call it but the kernel has the function pointer 
> >> for this structure set to NULL incorrectly.
> > 
> > This is a pretty vague description - what specific hardware 
> > is affected and how is the bug triggered?
> > 
> > Thanks,
> > 
> > 	Ingo
> > 
> Ingo,
>
> I am unaware of any hardware exactly that this effects. 
> However this should be fixed as a preventive feature in 
> order to avoid future bugs related to hardware that need 
> this function to be supported by the kernel. If you wish 
> I can rewrite the commit message stating this instead of 
> my earlier commit message.

Fair enough, and yes, adding that info would be nice - just 
appending your explanation would do the trick for me!

Thanks,

	Ingo

Re: [PATCH RESEND] x86:pci: Change sta2x11_dma_ops stucture to use switolb_dma_supported as it's dma_supported function in sta2x11-fixup.c

[Ingo Molnar]

* Nicholas Krause <xerofoify@gmail.com> wrote:

> 
> 
> On March 10, 2015 12:45:01 AM EDT, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >* Nicholas Krause <xerofoify@gmail.com> wrote:
> >
> >> This changes the structure sta2x11_dma_ops stucture to use
> >switolb_dma_supported as it's 
> >> function for dma_supported hardware verus setting this value to NULL
> >as this should be set 
> >> correctly for when dma_supported function needs to be called for
> >hardware needing this 
> >> function to be supported. Otherwise hardware needing this function to
> >be supported will 
> >> either cause a kernel panic or oops due to a NULL pointer being
> >dereferenced in the 
> >> structure, sta1x11_dma_ops for the dma_supported function. Further
> >more in  order to 
> >> prevent a kernel panic or oops here due to a NULL pointer being
> >deferred here we add the 
> >> function, swiotlb_dma_supported as the dma_supported function for the
> >structure,
> >> sta2x11_dma_ops.
> >
> >Pleasedontresendtotallyunstructuredandunreadablechangelogs.
> >
> >You also (still) don't explain where and how this supposed crash might 
> >happen in practice. Please don't resend unless you've addressed those 
> >deficiencies.
> >
> >I.e. I'm not convinced you know what you are modifying there and what 
> >effects your modifications will have.
> >
> >Thanks,
> >
> >	Ingo
> Ingo, 
> I was just wondering why my patch wasn't merged yet.  In addition this patch is more of a preventive measure as if there is no function pointer here either we kernel panic or oops at best if hardware needs to access a dma function for this structure .  At the moment I am unaware of any actual hardware that causes this issues, however this doesn't mean it won't happen at some point. I can rewrite my change log to this reasoning if required.
> Nick

1)

So as a reply to my feedback complaining about the quality of your 
submissions, you write an unstructured mail put into a single line 
with no newline at all? *whoosh*

2)

So I restructured your reply, added newlines at logical boundaries of 
thought, added proper punctuation and capitalization, fixed typos, 
removed phantom spaces - to make it minimally readable:

> I was just wondering why my patch wasn't merged yet.
>
> In addition, this patch is more of a preventive measure, as if there 
> is no function pointer here either we kernel panic or oops at best 
> if hardware needs to access a DMA function for this structure.
>
> At the moment I am unaware of any actual hardware that causes these 
> issues, however this doesn't mean it won't happen at some point.
>
> I can rewrite my change log to this reasoning if required.

You should have done this before expecting others to spend time on 
your mails.

3)

As to the technical substance of your patch: where exactly would the 
kernel panic or oops? Your changelog is missing actual analysis.

4)

And if you are wondering why maintainers have learned to start 
ignoring your trivial patches:

 - Unstructured changelogs, typos, missing punctuation, missing
   newlines, missing capitalization, phantom spaces cause submissions 
   to be ignored as trivially deficient.

 - No real analysis is found in your patch, beyond the line that was
   spewed out by 'git grep -i fixme'.

 - Mass mailing of borderline useless patches is a waste of time for
   everyone. There are thousands of trivial FIXME's in the kernel and
   you want to generate a commit for every single one?

The thing is, you now know how to write patches and how to upstream 
them. So it's time to rise beyond trivial patches: how about reading 
and understanding kernel code and fixing some real bugs?

5)

All in one, I'm not convinced you went beyond the 'git grep -i fixme' 
line in reading kernel code...

But it's up to Bjorn whether to merge your patch.

Thanks,

	Ingo

Re: [PATCH RESEND] x86:pci: Change sta2x11_dma_ops stucture to use switolb_dma_supported as it's dma_supported function in sta2x11-fixup.c

[Ingo Molnar]

* Nicholas Krause <xerofoify@gmail.com> wrote:

> This changes the structure sta2x11_dma_ops stucture to use switolb_dma_supported as it's 
> function for dma_supported hardware verus setting this value to NULL as this should be set 
> correctly for when dma_supported function needs to be called for hardware needing this 
> function to be supported. Otherwise hardware needing this function to be supported will 
> either cause a kernel panic or oops due to a NULL pointer being dereferenced in the 
> structure, sta1x11_dma_ops for the dma_supported function. Further more in  order to 
> prevent a kernel panic or oops here due to a NULL pointer being deferred here we add the 
> function, swiotlb_dma_supported as the dma_supported function for the structure,
> sta2x11_dma_ops.

Pleasedontresendtotallyunstructuredandunreadablechangelogs.

You also (still) don't explain where and how this supposed crash might 
happen in practice. Please don't resend unless you've addressed those 
deficiencies.

I.e. I'm not convinced you know what you are modifying there and what 
effects your modifications will have.

Thanks,

	Ingo