* Re: [PATCH splitout] mm: memory-failure: serialize TestSetPageHWPoison with zone->lock
From: Zi Yan @ 2026-06-09 18:52 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Andrew Morton, Michael S. Tsirkin, linux-kernel, Miaohe Lin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, Muchun Song,
Oscar Salvador, Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka,
Mike Rapoport, Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Baolin Wang, Nico Pache, Ryan Roberts, Dev Jain,
Barry Song, Lance Yang, Hugh Dickins, Matthew Brost, Joshua Hahn,
Rakie Kim, Byungchul Park, Gregory Price, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi
In-Reply-To: <FB250C43-790E-4AB0-BD40-74665FC601A0@nvidia.com>
On 9 Jun 2026, at 14:39, Zi Yan wrote:
> On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
>
>> On 6/9/26 20:10, Andrew Morton wrote:
>>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>
>>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
>>>> update to page->flags can race with non-atomic flag operations
>>>> that run under zone->lock in the buddy allocator.
>>>>
>>>> In particular, __free_pages_prepare() does:
>>>>
>>>> page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>
>>>> This non-atomic read-modify-write, while correctly excluding
>>>> __PG_HWPOISON from the mask, can still lose a concurrent
>>>> TestSetPageHWPoison if the read happens before the poison bit
>>>> is set and the write happens after. Will only get worse if/when
>>>> we add more non-atomic flag operations.
>>>>
>>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
>>>> around ClearPageHWPoison in the retry path. This
>>>> serializes with all buddy flag manipulation. The cost is
>>>> negligible: one lock/unlock in an extremely rare path
>>>> (hardware memory errors).
>>>>
>>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
>>>> in this file operate on pages already removed from the buddy
>>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
>>>> need zone->lock protection.
>>>
>>> Sashiko is saying this doesn't do anything "Because
>>> __free_pages_prepare() executes entirely locklessly". Did it goof?
>>>
>>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
>>
>> Battle of the bots: it's right.
>
> Yep, __free_pages_prepare() changes the page flag without holding
> zone->lock.
__free_pages_prepare() works on frozen pages and assumes no one else
touches the input page. To avoid this race, memory_failure() might
want to try_get_page() before TestClearPageHWPoison(), but I am not
sure if that works along with memory failure flow.
Best Regards,
Yan, Zi
^ permalink raw reply
* Re: [PATCH v4 01/47] x86/tsc: Never re-calibrate TSC frequency if its exact timing is known
From: Thomas Gleixner @ 2026-06-09 19:27 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov, Jan Kiszka,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
John Stultz, H. Peter Anvin, Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, kvm, linux-kernel, linux-coco, linux-hyperv,
virtualization, xen-devel, David Woodhouse, Tom Lendacky,
Nikunj A Dadhania, David Woodhouse, Michael Kelley
In-Reply-To: <aihKj-0nP7bUbNHH@google.com>
On Tue, Jun 09 2026 at 10:17, Sean Christopherson wrote:
> On Fri, Jun 05, 2026, Thomas Gleixner wrote:
>> On Fri, Jun 05 2026 at 11:04, Sean Christopherson wrote:
>> But we also should have a check in the TSC init code somewhere which
>> validates that X86_FEATURE_CONSTANT_TSC is set when
>> X86_FEATURE_TSC_KNOWN_FREQ is set. X86_FEATURE_TSC_KNOWN_FREQ is useless
>> w/o X86_FEATURE_CONSTANT_TSC.
>
> Ugh, any objection to punting on this for now? KVM and Xen guests will trigger
> TSC_KNOWN_FREQ without CONSTANT_TSC, thanks to commits:
>
> e10f78050323 ("kvmclock: fix TSC calibration for nested guests")
> 898ec52d2ba0 ("x86/xen/time: Set the X86_FEATURE_TSC_KNOWN_FREQ flag in xen_tsc_khz()")
>
> Hyper-V guests might as well? Hyper-V's handling of TSC is weird, even for a
> hypervisor.
Hypervisors are ranked by weirdness? I ranked them by insanity so far.
> Even when the frequency is provided in CPUID by the hypervisor, QEMU at least
> requires a fairly explicit opt-in to advertise CONSTANT_TSC, presumably to try
> to prevent users from shooting themselves in the foot.
Bah. We really should have enforced the dependency when we introduced
KNOWN_FREQ. But that ship has sailed.
Though for correctness sake this should be fixed at some point in the
foreseeable future.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v4 02/47] x86/tsc: Add a standalone helpers for getting TSC info from CPUID.0x15
From: Sean Christopherson @ 2026-06-09 19:28 UTC (permalink / raw)
To: Borislav Petkov
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Dave Hansen, x86,
Kiryl Shutsemau, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov, Jan Kiszka,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
John Stultz, H. Peter Anvin, Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, kvm, linux-kernel, linux-coco, linux-hyperv,
virtualization, xen-devel, David Woodhouse, Tom Lendacky,
Nikunj A Dadhania, David Woodhouse, Michael Kelley,
Thomas Gleixner
In-Reply-To: <20260602034916.GGah5SvARd77mkvxe3@fat_crate.local>
On Mon, Jun 01, 2026, Borislav Petkov wrote:
> On Fri, May 29, 2026 at 07:43:49AM -0700, Sean Christopherson wrote:
> > +static int cpuid_get_tsc_info(struct cpuid_tsc_info *info)
> > +{
> > + unsigned int ecx_hz, edx;
> > +
> > + memset(info, 0, sizeof(*info));
>
> Let's not clear this unnecessarily...
>
> > +
> > + if (boot_cpu_data.cpuid_level < CPUID_LEAF_TSC)
> > + return -ENOENT;
>
> ... just to return here...
>
> > +
> > + /* CPUID 15H TSC/Crystal ratio, plus optionally Crystal Hz */
> > + cpuid(CPUID_LEAF_TSC, &info->denominator, &info->numerator, &ecx_hz, &edx);
> > +
> > + if (!info->denominator || !info->numerator)
> > + return -ENOENT;
>
> ... or here.
>
> We wanna clear it here, when we'll return success.
Actually, if we take the approach of relying on the user to check the return
code, then there's no need to zero the struct since all fields will be explicitly
written, especially if we drop the "tsc_khz" field. I was zeroing the field
purely as defense in depth.
^ permalink raw reply
* Re: [PATCH splitout] mm: page_reporting: allow driver to set batch capacity
From: Michael S. Tsirkin @ 2026-06-09 20:08 UTC (permalink / raw)
To: Gregory Price
Cc: linux-kernel, Miaohe Lin, David Hildenbrand (Arm), Jason Wang,
Xuan Zhuo, Eugenio Pérez, Muchun Song, Oscar Salvador,
Andrew Morton, Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka,
Mike Rapoport, Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Zi Yan, Baolin Wang, Nico Pache, Ryan Roberts,
Dev Jain, Barry Song, Lance Yang, Hugh Dickins, Matthew Brost,
Joshua Hahn, Rakie Kim, Byungchul Park, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi,
Alexander Duyck
In-Reply-To: <aihRBTaTUmgYmZfX@gourry-fedora-PF4VCD3F>
On Tue, Jun 09, 2026 at 01:44:37PM -0400, Gregory Price wrote:
> On Tue, Jun 09, 2026 at 11:53:20AM -0400, Michael S. Tsirkin wrote:
> > --- a/mm/page_reporting.c
> > +++ b/mm/page_reporting.c
> > @@ -174,10 +174,10 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
> > * list processed. This should result in us reporting all pages on
> > * an idle system in about 30 seconds.
> > *
> > - * The division here should be cheap since PAGE_REPORTING_CAPACITY
> > - * should always be a power of 2.
> > + * The division here uses integer division; capacity need
> > + * not be a power of 2.
> > */
> > - budget = DIV_ROUND_UP(area->nr_free, PAGE_REPORTING_CAPACITY * 16);
> > + budget = DIV_ROUND_UP(area->nr_free, prdev->capacity * 16);
> >
>
> Initial look - is there a div-by-0 here? I noticed the old check
> prevents this from being (0 * 16), but i don't see (on first pass)
> the same check anywhere.
>
> Unless this line below always forces the above to be a
> PAGE_REPORTING_CAPCAITY if it's set to 0.
It does, does it not?
> > + if (!prdev->capacity || prdev->capacity > PAGE_REPORTING_CAPACITY)
> > + prdev->capacity = PAGE_REPORTING_CAPACITY;
> > +
>
> It's worth making this corner condition a little more obvious.
>
> The code intends for
>
> if (capacity == 0)
> capacity = PAGE_REPORTING_CAPACITY
>
> but that's not reflected in the changelog as a default value.
>
> When happens if a driver sets (capacity=0) either on purpose (???)
what would the purpose be? if you don't want reporting do not register.
> or
> because there's a bug (???)
exactly ??? since where are we practicing defensive programming in kernel
APIs?
> and then page_reporting.c forces it up to
> 32?
>
> There's something to improve here.
>
> ~Gregory
So I'll update the commit log to mention PAGE_REPORTING_CAPACITY.
And maybe a comment near capacity field?
Should be enough?
--
MST
^ permalink raw reply
* Re: [PATCH splitout] mm: memory-failure: serialize TestSetPageHWPoison with zone->lock
From: Michael S. Tsirkin @ 2026-06-09 20:24 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Miaohe Lin, David Hildenbrand (Arm), Jason Wang,
Xuan Zhuo, Eugenio Pérez, Muchun Song, Oscar Salvador,
Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Zi Yan, Baolin Wang, Nico Pache, Ryan Roberts,
Dev Jain, Barry Song, Lance Yang, Hugh Dickins, Matthew Brost,
Joshua Hahn, Rakie Kim, Byungchul Park, Gregory Price, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi
In-Reply-To: <20260609111020.e88f51a7b6ebc37360d66fdc@linux-foundation.org>
On Tue, Jun 09, 2026 at 11:10:20AM -0700, Andrew Morton wrote:
> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > TestSetPageHWPoison() is called without zone->lock, so its atomic
> > update to page->flags can race with non-atomic flag operations
> > that run under zone->lock in the buddy allocator.
> >
> > In particular, __free_pages_prepare() does:
> >
> > page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> >
> > This non-atomic read-modify-write, while correctly excluding
> > __PG_HWPOISON from the mask, can still lose a concurrent
> > TestSetPageHWPoison if the read happens before the poison bit
> > is set and the write happens after. Will only get worse if/when
> > we add more non-atomic flag operations.
> >
> > Fix by acquiring zone->lock around TestSetPageHWPoison and
> > around ClearPageHWPoison in the retry path. This
> > serializes with all buddy flag manipulation. The cost is
> > negligible: one lock/unlock in an extremely rare path
> > (hardware memory errors).
> >
> > Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> > in this file operate on pages already removed from the buddy
> > allocator or on non-buddy pages (DAX, hugetlb), so they do not
> > need zone->lock protection.
>
> Sashiko is saying this doesn't do anything "Because
> __free_pages_prepare() executes entirely locklessly". Did it goof?
>
> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
>
Oh. So it only helps with the prezero patches. Maybe other places where
flags are touched locklessly. Not __free_pages_prepare. I was too
focused on that. Scrap this please. I'll try to think of something.
--
MST
^ permalink raw reply
* Re: [PATCH splitout] mm: memory-failure: serialize TestSetPageHWPoison with zone->lock
From: Michael S. Tsirkin @ 2026-06-09 20:34 UTC (permalink / raw)
To: Zi Yan
Cc: David Hildenbrand (Arm), Andrew Morton, linux-kernel, Miaohe Lin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, Muchun Song,
Oscar Salvador, Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka,
Mike Rapoport, Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Baolin Wang, Nico Pache, Ryan Roberts, Dev Jain,
Barry Song, Lance Yang, Hugh Dickins, Matthew Brost, Joshua Hahn,
Rakie Kim, Byungchul Park, Gregory Price, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi
In-Reply-To: <38C84F23-E881-4DB2-86BA-93F39D44AE1B@nvidia.com>
On Tue, Jun 09, 2026 at 02:52:47PM -0400, Zi Yan wrote:
> On 9 Jun 2026, at 14:39, Zi Yan wrote:
>
> > On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
> >
> >> On 6/9/26 20:10, Andrew Morton wrote:
> >>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>
> >>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
> >>>> update to page->flags can race with non-atomic flag operations
> >>>> that run under zone->lock in the buddy allocator.
> >>>>
> >>>> In particular, __free_pages_prepare() does:
> >>>>
> >>>> page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> >>>>
> >>>> This non-atomic read-modify-write, while correctly excluding
> >>>> __PG_HWPOISON from the mask, can still lose a concurrent
> >>>> TestSetPageHWPoison if the read happens before the poison bit
> >>>> is set and the write happens after. Will only get worse if/when
> >>>> we add more non-atomic flag operations.
> >>>>
> >>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
> >>>> around ClearPageHWPoison in the retry path. This
> >>>> serializes with all buddy flag manipulation. The cost is
> >>>> negligible: one lock/unlock in an extremely rare path
> >>>> (hardware memory errors).
> >>>>
> >>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> >>>> in this file operate on pages already removed from the buddy
> >>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
> >>>> need zone->lock protection.
> >>>
> >>> Sashiko is saying this doesn't do anything "Because
> >>> __free_pages_prepare() executes entirely locklessly". Did it goof?
> >>>
> >>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
> >>
> >> Battle of the bots: it's right.
> >
> > Yep, __free_pages_prepare() changes the page flag without holding
> > zone->lock.
>
> __free_pages_prepare() works on frozen pages and assumes no one else
> touches the input page. To avoid this race, memory_failure() might
> want to try_get_page() before TestClearPageHWPoison(), but I am not
> sure if that works along with memory failure flow.
>
> Best Regards,
> Yan, Zi
Actually memory failure already plays with this down the road no?
So maybe it's enough to just SetPageHWPoison afterwards again?
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index ee42d4361309..4758fea94a96 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2415,6 +2415,7 @@ int memory_failure(unsigned long pfn, int flags)
if (!res) {
if (is_free_buddy_page(p)) {
if (take_page_off_buddy(p)) {
+ SetPageHWPoison(p);
page_ref_inc(p);
res = MF_RECOVERED;
} else {
and maybe in a bunch of other places in there?
--
MST
^ permalink raw reply related
* Re: [PATCH splitout] mm: memory-failure: serialize TestSetPageHWPoison with zone->lock
From: Zi Yan @ 2026-06-09 20:54 UTC (permalink / raw)
To: Michael S. Tsirkin, Miaohe Lin
Cc: David Hildenbrand (Arm), Andrew Morton, linux-kernel, Jason Wang,
Xuan Zhuo, Eugenio Pérez, Muchun Song, Oscar Salvador,
Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Baolin Wang, Nico Pache, Ryan Roberts, Dev Jain,
Barry Song, Lance Yang, Hugh Dickins, Matthew Brost, Joshua Hahn,
Rakie Kim, Byungchul Park, Gregory Price, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi
In-Reply-To: <20260609162437-mutt-send-email-mst@kernel.org>
On 9 Jun 2026, at 16:34, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2026 at 02:52:47PM -0400, Zi Yan wrote:
>> On 9 Jun 2026, at 14:39, Zi Yan wrote:
>>
>>> On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
>>>
>>>> On 6/9/26 20:10, Andrew Morton wrote:
>>>>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>>
>>>>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
>>>>>> update to page->flags can race with non-atomic flag operations
>>>>>> that run under zone->lock in the buddy allocator.
>>>>>>
>>>>>> In particular, __free_pages_prepare() does:
>>>>>>
>>>>>> page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>>>
>>>>>> This non-atomic read-modify-write, while correctly excluding
>>>>>> __PG_HWPOISON from the mask, can still lose a concurrent
>>>>>> TestSetPageHWPoison if the read happens before the poison bit
>>>>>> is set and the write happens after. Will only get worse if/when
>>>>>> we add more non-atomic flag operations.
>>>>>>
>>>>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
>>>>>> around ClearPageHWPoison in the retry path. This
>>>>>> serializes with all buddy flag manipulation. The cost is
>>>>>> negligible: one lock/unlock in an extremely rare path
>>>>>> (hardware memory errors).
>>>>>>
>>>>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
>>>>>> in this file operate on pages already removed from the buddy
>>>>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
>>>>>> need zone->lock protection.
>>>>>
>>>>> Sashiko is saying this doesn't do anything "Because
>>>>> __free_pages_prepare() executes entirely locklessly". Did it goof?
>>>>>
>>>>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
>>>>
>>>> Battle of the bots: it's right.
>>>
>>> Yep, __free_pages_prepare() changes the page flag without holding
>>> zone->lock.
>>
>> __free_pages_prepare() works on frozen pages and assumes no one else
>> touches the input page. To avoid this race, memory_failure() might
>> want to try_get_page() before TestClearPageHWPoison(), but I am not
>> sure if that works along with memory failure flow.
>>
>> Best Regards,
>> Yan, Zi
>
>
>
> Actually memory failure already plays with this down the road no?
>
> So maybe it's enough to just SetPageHWPoison afterwards again?
>
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index ee42d4361309..4758fea94a96 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2415,6 +2415,7 @@ int memory_failure(unsigned long pfn, int flags)
> if (!res) {
> if (is_free_buddy_page(p)) {
> if (take_page_off_buddy(p)) {
> + SetPageHWPoison(p);
> page_ref_inc(p);
> res = MF_RECOVERED;
> } else {
>
>
> and maybe in a bunch of other places in there?
You mean for fear of losing HWPoison flag in the earlier TestSetPageHWPoison(),
just set it again here? Why not do it after get_hwpoison_page(), since that
is the expected page flag? Miaohe probably can give a better answer here.
Best Regards,
Yan, Zi
^ permalink raw reply
* Re: [PATCH splitout] mm: memory-failure: serialize TestSetPageHWPoison with zone->lock
From: Michael S. Tsirkin @ 2026-06-09 21:00 UTC (permalink / raw)
To: Zi Yan
Cc: Miaohe Lin, David Hildenbrand (Arm), Andrew Morton, linux-kernel,
Jason Wang, Xuan Zhuo, Eugenio Pérez, Muchun Song,
Oscar Salvador, Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka,
Mike Rapoport, Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Baolin Wang, Nico Pache, Ryan Roberts, Dev Jain,
Barry Song, Lance Yang, Hugh Dickins, Matthew Brost, Joshua Hahn,
Rakie Kim, Byungchul Park, Gregory Price, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi
In-Reply-To: <4BA276D9-9EB9-4E2A-8A05-657ACACFF227@nvidia.com>
On Tue, Jun 09, 2026 at 04:54:01PM -0400, Zi Yan wrote:
> On 9 Jun 2026, at 16:34, Michael S. Tsirkin wrote:
>
> > On Tue, Jun 09, 2026 at 02:52:47PM -0400, Zi Yan wrote:
> >> On 9 Jun 2026, at 14:39, Zi Yan wrote:
> >>
> >>> On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
> >>>
> >>>> On 6/9/26 20:10, Andrew Morton wrote:
> >>>>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>>>
> >>>>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
> >>>>>> update to page->flags can race with non-atomic flag operations
> >>>>>> that run under zone->lock in the buddy allocator.
> >>>>>>
> >>>>>> In particular, __free_pages_prepare() does:
> >>>>>>
> >>>>>> page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> >>>>>>
> >>>>>> This non-atomic read-modify-write, while correctly excluding
> >>>>>> __PG_HWPOISON from the mask, can still lose a concurrent
> >>>>>> TestSetPageHWPoison if the read happens before the poison bit
> >>>>>> is set and the write happens after. Will only get worse if/when
> >>>>>> we add more non-atomic flag operations.
> >>>>>>
> >>>>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
> >>>>>> around ClearPageHWPoison in the retry path. This
> >>>>>> serializes with all buddy flag manipulation. The cost is
> >>>>>> negligible: one lock/unlock in an extremely rare path
> >>>>>> (hardware memory errors).
> >>>>>>
> >>>>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> >>>>>> in this file operate on pages already removed from the buddy
> >>>>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
> >>>>>> need zone->lock protection.
> >>>>>
> >>>>> Sashiko is saying this doesn't do anything "Because
> >>>>> __free_pages_prepare() executes entirely locklessly". Did it goof?
> >>>>>
> >>>>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
> >>>>
> >>>> Battle of the bots: it's right.
> >>>
> >>> Yep, __free_pages_prepare() changes the page flag without holding
> >>> zone->lock.
> >>
> >> __free_pages_prepare() works on frozen pages and assumes no one else
> >> touches the input page. To avoid this race, memory_failure() might
> >> want to try_get_page() before TestClearPageHWPoison(), but I am not
> >> sure if that works along with memory failure flow.
> >>
> >> Best Regards,
> >> Yan, Zi
> >
> >
> >
> > Actually memory failure already plays with this down the road no?
> >
> > So maybe it's enough to just SetPageHWPoison afterwards again?
> >
> >
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index ee42d4361309..4758fea94a96 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -2415,6 +2415,7 @@ int memory_failure(unsigned long pfn, int flags)
> > if (!res) {
> > if (is_free_buddy_page(p)) {
> > if (take_page_off_buddy(p)) {
> > + SetPageHWPoison(p);
> > page_ref_inc(p);
> > res = MF_RECOVERED;
> > } else {
> >
> >
> > and maybe in a bunch of other places in there?
>
> You mean for fear of losing HWPoison flag in the earlier TestSetPageHWPoison(),
> just set it again here?
Yea.
> Why not do it after get_hwpoison_page(), since that
> is the expected page flag?
It's still in the buddy at that point right? I'm worried buddy might
poke at flags.
> Miaohe probably can give a better answer here.
>
>
> Best Regards,
> Yan, Zi
^ permalink raw reply
* Re: [PATCH splitout] mm: page_reporting: allow driver to set batch capacity
From: Gregory Price @ 2026-06-09 21:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Miaohe Lin, David Hildenbrand (Arm), Jason Wang,
Xuan Zhuo, Eugenio Pérez, Muchun Song, Oscar Salvador,
Andrew Morton, Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka,
Mike Rapoport, Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Zi Yan, Baolin Wang, Nico Pache, Ryan Roberts,
Dev Jain, Barry Song, Lance Yang, Hugh Dickins, Matthew Brost,
Joshua Hahn, Rakie Kim, Byungchul Park, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi,
Alexander Duyck
In-Reply-To: <20260609160506-mutt-send-email-mst@kernel.org>
On Tue, Jun 09, 2026 at 04:08:08PM -0400, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2026 at 01:44:37PM -0400, Gregory Price wrote:
> > On Tue, Jun 09, 2026 at 11:53:20AM -0400, Michael S. Tsirkin wrote:
> > > --- a/mm/page_reporting.c
> > > +++ b/mm/page_reporting.c
> > > @@ -174,10 +174,10 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
> > > * list processed. This should result in us reporting all pages on
> > > * an idle system in about 30 seconds.
> > > *
> > > - * The division here should be cheap since PAGE_REPORTING_CAPACITY
> > > - * should always be a power of 2.
> > > + * The division here uses integer division; capacity need
> > > + * not be a power of 2.
> > > */
> > > - budget = DIV_ROUND_UP(area->nr_free, PAGE_REPORTING_CAPACITY * 16);
> > > + budget = DIV_ROUND_UP(area->nr_free, prdev->capacity * 16);
> > >
> >
> > Initial look - is there a div-by-0 here? I noticed the old check
> > prevents this from being (0 * 16), but i don't see (on first pass)
> > the same check anywhere.
> >
> > Unless this line below always forces the above to be a
> > PAGE_REPORTING_CAPCAITY if it's set to 0.
>
> It does, does it not?
>
> > > + if (!prdev->capacity || prdev->capacity > PAGE_REPORTING_CAPACITY)
> > > + prdev->capacity = PAGE_REPORTING_CAPACITY;
> > > +
> >
> > It's worth making this corner condition a little more obvious.
> >
> > The code intends for
> >
> > if (capacity == 0)
> > capacity = PAGE_REPORTING_CAPACITY
> >
> > but that's not reflected in the changelog as a default value.
> >
> > When happens if a driver sets (capacity=0) either on purpose (???)
>
> what would the purpose be? if you don't want reporting do not register.
>
> > or
> > because there's a bug (???)
>
> exactly ??? since where are we practicing defensive programming in kernel
> APIs?
>
> > and then page_reporting.c forces it up to
> > 32?
> >
> > There's something to improve here.
> >
> > ~Gregory
>
>
> So I'll update the commit log to mention PAGE_REPORTING_CAPACITY.
> And maybe a comment near capacity field?
> Should be enough?
I suppose the question is whether capacity=0 should cause a WARN (i.e.
only a bug explains that value), or if capacity=0 means something
special (i.e. use the default) and therefore that should be documented.
I don't know which of these is the case, but if it's the latter than
that deserves a comment yes.
~Gregory
^ permalink raw reply
* Re: [PATCH splitout] mm: page_reporting: allow driver to set batch capacity
From: Michael S. Tsirkin @ 2026-06-09 22:00 UTC (permalink / raw)
To: Gregory Price
Cc: linux-kernel, Miaohe Lin, David Hildenbrand (Arm), Jason Wang,
Xuan Zhuo, Eugenio Pérez, Muchun Song, Oscar Salvador,
Andrew Morton, Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka,
Mike Rapoport, Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Zi Yan, Baolin Wang, Nico Pache, Ryan Roberts,
Dev Jain, Barry Song, Lance Yang, Hugh Dickins, Matthew Brost,
Joshua Hahn, Rakie Kim, Byungchul Park, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi,
Alexander Duyck
In-Reply-To: <aiiJPwhn6gF3ILur@gourry-fedora-PF4VCD3F>
On Tue, Jun 09, 2026 at 05:44:31PM -0400, Gregory Price wrote:
> On Tue, Jun 09, 2026 at 04:08:08PM -0400, Michael S. Tsirkin wrote:
> > On Tue, Jun 09, 2026 at 01:44:37PM -0400, Gregory Price wrote:
> > > On Tue, Jun 09, 2026 at 11:53:20AM -0400, Michael S. Tsirkin wrote:
> > > > --- a/mm/page_reporting.c
> > > > +++ b/mm/page_reporting.c
> > > > @@ -174,10 +174,10 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
> > > > * list processed. This should result in us reporting all pages on
> > > > * an idle system in about 30 seconds.
> > > > *
> > > > - * The division here should be cheap since PAGE_REPORTING_CAPACITY
> > > > - * should always be a power of 2.
> > > > + * The division here uses integer division; capacity need
> > > > + * not be a power of 2.
> > > > */
> > > > - budget = DIV_ROUND_UP(area->nr_free, PAGE_REPORTING_CAPACITY * 16);
> > > > + budget = DIV_ROUND_UP(area->nr_free, prdev->capacity * 16);
> > > >
> > >
> > > Initial look - is there a div-by-0 here? I noticed the old check
> > > prevents this from being (0 * 16), but i don't see (on first pass)
> > > the same check anywhere.
> > >
> > > Unless this line below always forces the above to be a
> > > PAGE_REPORTING_CAPCAITY if it's set to 0.
> >
> > It does, does it not?
> >
> > > > + if (!prdev->capacity || prdev->capacity > PAGE_REPORTING_CAPACITY)
> > > > + prdev->capacity = PAGE_REPORTING_CAPACITY;
> > > > +
> > >
> > > It's worth making this corner condition a little more obvious.
> > >
> > > The code intends for
> > >
> > > if (capacity == 0)
> > > capacity = PAGE_REPORTING_CAPACITY
> > >
> > > but that's not reflected in the changelog as a default value.
> > >
> > > When happens if a driver sets (capacity=0) either on purpose (???)
> >
> > what would the purpose be? if you don't want reporting do not register.
> >
> > > or
> > > because there's a bug (???)
> >
> > exactly ??? since where are we practicing defensive programming in kernel
> > APIs?
> >
> > > and then page_reporting.c forces it up to
> > > 32?
> > >
> > > There's something to improve here.
> > >
> > > ~Gregory
> >
> >
> > So I'll update the commit log to mention PAGE_REPORTING_CAPACITY.
> > And maybe a comment near capacity field?
> > Should be enough?
>
> I suppose the question is whether capacity=0 should cause a WARN (i.e.
> only a bug explains that value), or if capacity=0 means something
> special (i.e. use the default) and therefore that should be documented.
> I don't know which of these is the case, but if it's the latter than
> that deserves a comment yes.
>
> ~Gregory
It's the default. Will document.
^ permalink raw reply
* Re: [PATCH] [v3] vduse: fix compat handling for VDUSE_IOTLB_GET_FD/VDUSE_VQ_GET_INFO
From: Michael S. Tsirkin @ 2026-06-10 5:26 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jason Wang, Xie Yongji, Arnd Bergmann, Eugenio Pérez,
Xuan Zhuo, Marco Crivellari, Anders Roxell, virtualization,
linux-kernel
In-Reply-To: <20260213154051.4172275-1-arnd@kernel.org>
On Fri, Feb 13, 2026 at 04:40:46PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> These two ioctls are incompatible on 32-bit x86 userspace, because
> the data structures are shorter than they are on 64-bit.
>
> Add a proper .compat_ioctl handler for x86 that reads the structures
> with the smaller padding before calling the internal handlers. On
> all other architectures, CONFIG_COMPAT_FOR_U64_ALIGNMENT is disabled
> and no special handling is required.
>
> Fixes: ad146355bfad ("vduse: Support querying information of IOVA regions")
> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v3 changes:
> - check CONFIG_COMPAT_FOR_U64_ALIGNMENT in preprocessor
> v2 changes:
> - split compat handler into separate function
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 123 ++++++++++++++++++++++++++++-
> 1 file changed, 122 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 405d59610f76..e0f5a7397221 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1618,6 +1618,127 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> return ret;
> }
>
> +#ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT
> +/*
> + * i386 has different alignment constraints than x86_64,
> + * so there are only 3 bytes of padding instead of 7.
> + */
> +struct compat_vduse_iotlb_entry {
> + compat_u64 offset;
> + compat_u64 start;
> + compat_u64 last;
> + __u8 perm;
> + __u8 padding[3];
> +};
> +#define COMPAT_VDUSE_IOTLB_GET_FD _IOWR(VDUSE_BASE, 0x10, struct compat_vduse_iotlb_entry)
> +
> +struct compat_vduse_vq_info {
> + __u32 index;
> + __u32 num;
> + compat_u64 desc_addr;
> + compat_u64 driver_addr;
> + compat_u64 device_addr;
> + union {
> + struct vduse_vq_state_split split;
> + struct vduse_vq_state_packed packed;
> + };
> + __u8 ready;
> + __u8 padding[3];
> +} __uapi_arch_align;
what is this __uapi_arch_align supposed to be doing?
It compiles by luck because gcc thinks it's a global variable.
> +#define COMPAT_VDUSE_VQ_GET_INFO _IOWR(VDUSE_BASE, 0x15, struct compat_vduse_vq_info)
> +
> +static long vduse_dev_compat_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct vduse_dev *dev = file->private_data;
> + void __user *argp = (void __user *)arg;
> + int ret;
> +
> + if (unlikely(dev->broken))
> + return -EPERM;
> +
> + switch (cmd) {
> + case COMPAT_VDUSE_IOTLB_GET_FD: {
> + struct vduse_iotlb_entry_v2 entry = {0};
> + struct file *f = NULL;
> +
> + ret = -EFAULT;
> + if (copy_from_user(&entry, argp, _IOC_SIZE(cmd)))
> + break;
> +
> + ret = vduse_dev_iotlb_entry(dev, &entry, &f, NULL);
> + if (ret)
> + break;
> +
> + ret = -EINVAL;
> + if (!f)
> + break;
> +
> + ret = copy_to_user(argp, &entry, _IOC_SIZE(cmd));
> + if (ret) {
> + ret = -EFAULT;
> + fput(f);
> + break;
> + }
> + ret = receive_fd(f, NULL, perm_to_file_flags(entry.perm));
> + fput(f);
> + break;
> + }
> + case COMPAT_VDUSE_VQ_GET_INFO: {
> + struct vduse_vq_info vq_info = {};
> + struct vduse_virtqueue *vq;
> + u32 index;
> +
> + ret = -EFAULT;
> + if (copy_from_user(&vq_info, argp,
> + sizeof(struct compat_vduse_vq_info)))
> + break;
> +
> + ret = -EINVAL;
> + if (vq_info.index >= dev->vq_num)
> + break;
> +
> + index = array_index_nospec(vq_info.index, dev->vq_num);
> + vq = dev->vqs[index];
> + vq_info.desc_addr = vq->desc_addr;
> + vq_info.driver_addr = vq->driver_addr;
> + vq_info.device_addr = vq->device_addr;
> + vq_info.num = vq->num;
> +
> + if (dev->driver_features & BIT_ULL(VIRTIO_F_RING_PACKED)) {
> + vq_info.packed.last_avail_counter =
> + vq->state.packed.last_avail_counter;
> + vq_info.packed.last_avail_idx =
> + vq->state.packed.last_avail_idx;
> + vq_info.packed.last_used_counter =
> + vq->state.packed.last_used_counter;
> + vq_info.packed.last_used_idx =
> + vq->state.packed.last_used_idx;
> + } else
> + vq_info.split.avail_index =
> + vq->state.split.avail_index;
> +
> + vq_info.ready = vq->ready;
> +
> + ret = -EFAULT;
> + if (copy_to_user(argp, &vq_info,
> + sizeof(struct compat_vduse_vq_info)))
> + break;
> +
> + ret = 0;
> + break;
> + }
> + default:
> + ret = -ENOIOCTLCMD;
> + break;
> + }
> +
> + return vduse_dev_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> +}
> +#else
> +#define vduse_dev_compat_ioctl compat_ptr_ioctl
> +#endif
> +
> static int vduse_dev_release(struct inode *inode, struct file *file)
> {
> struct vduse_dev *dev = file->private_data;
> @@ -1678,7 +1799,7 @@ static const struct file_operations vduse_dev_fops = {
> .write_iter = vduse_dev_write_iter,
> .poll = vduse_dev_poll,
> .unlocked_ioctl = vduse_dev_ioctl,
> - .compat_ioctl = compat_ptr_ioctl,
> + .compat_ioctl = vduse_dev_compat_ioctl,
> .llseek = noop_llseek,
> };
>
> --
> 2.39.5
^ permalink raw reply
* Re: [PATCH v3 2/4] scsi: host: allocate struct Scsi_Host on the NUMA node of the host adapter
From: Hannes Reinecke @ 2026-06-10 5:59 UTC (permalink / raw)
To: Sumit Saxena, Martin K . Petersen, Jens Axboe
Cc: James E . J . Bottomley, linux-scsi, linux-block, Adam Radford,
Khalid Aziz, Adaptec OEM Raid Solutions, Matthew Wilcox,
Juergen E . Fischer, Russell King, linux-arm-kernel, Finn Thain,
Michael Schmitz, Anil Gurumurthy, Sudarsana Kalluru,
Oliver Neukum, Ali Akcaagac, Jamie Lenehan, Ram Vegesna,
target-devel, Bradley Grove, Satish Kharat, Sesidhar Baddela,
Karan Tilak Kumar, Yihang Li, Don Brace, storagedev,
HighPoint Linux Team, Tyrel Datwyler, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, linuxppc-dev,
Brian King, Lee Duncan, Chris Leech, Mike Christie, open-iscsi,
Justin Tee, Paul Ely, Kashyap Desai, Shivasharan S,
Chandrakanth Patil, megaraidlinux.pdl, Sathya Prakash Veerichetty,
Sreekanth Reddy, mpi3mr-linuxdrv.pdl, Suganath Prabu Subramani,
Ranjan Kumar, MPT-FusionLinux.pdl, Daniel Palmer, GOTO Masanori,
YOKOTA Hiroshi, Jack Wang, Geoff Levand, Michael Reed,
Nilesh Javali, GR-QLogic-Storage-Upstream, Narsimhulu Musini,
K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
linux-hyperv, Michael S . Tsirkin, Jason Wang, Paolo Bonzini,
Stefan Hajnoczi, Eugenio Perez, virtualization, Vishal Bhakta,
bcm-kernel-feedback-list, Juergen Gross, Stefano Stabellini,
Oleksandr Tyshchenko, xen-devel, John Garry
In-Reply-To: <20260609121806.2121755-3-sumit.saxena@broadcom.com>
On 6/9/26 14:18, Sumit Saxena wrote:
> scsi_host_alloc() used kzalloc(), which always picks an arbitrary node.
> Extend the function to accept a 'struct device *dev' parameter and use
> kzalloc_node() with dev_to_node(dev) so the Scsi_Host struct lands on
> the same NUMA node as the HBA, mirroring the treatment already applied
> to struct scsi_device, struct scsi_target, and shost_data.
>
> When dev is NULL (legacy ISA/platform drivers without a dma_dev) the
> allocation falls back to NUMA_NO_NODE, preserving existing behaviour.
>
> Update all in-tree callers:
> - PCI-based HBA drivers pass &pdev->dev (or the equivalent struct
> member such as &phba->pcidev->dev, &h->pdev->dev, &ha->pdev->dev)
> so their host struct is placed on the adapter's node.
> - Non-PCI drivers (ISA, Amiga, ARM PCMCIA, virtio, Hyper-V, PS3, …)
> pass NULL.
> - libfc's libfc_host_alloc() inline helper passes NULL; FC drivers
> that want NUMA awareness can open-code the call with their pdev.
>
> Suggested-by: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> ---
> drivers/scsi/3w-9xxx.c | 2 +-
> drivers/scsi/3w-sas.c | 2 +-
> drivers/scsi/3w-xxxx.c | 2 +-
> drivers/scsi/53c700.c | 2 +-
> drivers/scsi/BusLogic.c | 2 +-
> drivers/scsi/a100u2w.c | 2 +-
> drivers/scsi/a2091.c | 2 +-
> drivers/scsi/a3000.c | 2 +-
> drivers/scsi/aacraid/linit.c | 2 +-
> drivers/scsi/advansys.c | 6 +++---
> drivers/scsi/aha152x.c | 2 +-
> drivers/scsi/aha1542.c | 2 +-
> drivers/scsi/aha1740.c | 2 +-
> drivers/scsi/aic7xxx/aic79xx_osm.c | 2 +-
> drivers/scsi/aic7xxx/aic7xxx_osm.c | 2 +-
> drivers/scsi/aic94xx/aic94xx_init.c | 2 +-
> drivers/scsi/am53c974.c | 2 +-
> drivers/scsi/arcmsr/arcmsr_hba.c | 3 ++-
> drivers/scsi/arm/acornscsi.c | 2 +-
> drivers/scsi/arm/arxescsi.c | 2 +-
> drivers/scsi/arm/cumana_1.c | 2 +-
> drivers/scsi/arm/cumana_2.c | 2 +-
> drivers/scsi/arm/eesox.c | 2 +-
> drivers/scsi/arm/oak.c | 2 +-
> drivers/scsi/arm/powertec.c | 2 +-
> drivers/scsi/atari_scsi.c | 2 +-
> drivers/scsi/atp870u.c | 2 +-
> drivers/scsi/bfa/bfad_im.c | 2 +-
> drivers/scsi/csiostor/csio_init.c | 4 ++--
> drivers/scsi/dc395x.c | 2 +-
> drivers/scsi/dmx3191d.c | 2 +-
> drivers/scsi/elx/efct/efct_xport.c | 4 ++--
> drivers/scsi/esas2r/esas2r_main.c | 2 +-
> drivers/scsi/fdomain.c | 2 +-
> drivers/scsi/fnic/fnic_main.c | 2 +-
> drivers/scsi/g_NCR5380.c | 2 +-
> drivers/scsi/gvp11.c | 2 +-
> drivers/scsi/hisi_sas/hisi_sas_main.c | 2 +-
> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 2 +-
> drivers/scsi/hosts.c | 6 ++++--
> drivers/scsi/hpsa.c | 2 +-
> drivers/scsi/hptiop.c | 2 +-
> drivers/scsi/ibmvscsi/ibmvfc.c | 2 +-
> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
> drivers/scsi/imm.c | 2 +-
> drivers/scsi/initio.c | 2 +-
> drivers/scsi/ipr.c | 2 +-
> drivers/scsi/ips.c | 2 +-
> drivers/scsi/isci/init.c | 2 +-
> drivers/scsi/jazz_esp.c | 2 +-
> drivers/scsi/libiscsi.c | 2 +-
> drivers/scsi/lpfc/lpfc_init.c | 2 +-
> drivers/scsi/mac53c94.c | 2 +-
> drivers/scsi/mac_esp.c | 2 +-
> drivers/scsi/mac_scsi.c | 2 +-
> drivers/scsi/megaraid.c | 2 +-
> drivers/scsi/megaraid/megaraid_mbox.c | 2 +-
> drivers/scsi/megaraid/megaraid_sas_base.c | 2 +-
> drivers/scsi/mesh.c | 2 +-
> drivers/scsi/mpi3mr/mpi3mr_os.c | 2 +-
> drivers/scsi/mpt3sas/mpt3sas_scsih.c | 4 ++--
> drivers/scsi/mvme147.c | 2 +-
> drivers/scsi/mvsas/mv_init.c | 2 +-
> drivers/scsi/mvumi.c | 2 +-
> drivers/scsi/myrb.c | 2 +-
> drivers/scsi/myrs.c | 2 +-
> drivers/scsi/ncr53c8xx.c | 2 +-
> drivers/scsi/nsp32.c | 2 +-
> drivers/scsi/pcmcia/nsp_cs.c | 2 +-
> drivers/scsi/pcmcia/qlogic_stub.c | 2 +-
> drivers/scsi/pcmcia/sym53c500_cs.c | 2 +-
> drivers/scsi/pm8001/pm8001_init.c | 2 +-
> drivers/scsi/pmcraid.c | 2 +-
> drivers/scsi/ppa.c | 2 +-
> drivers/scsi/ps3rom.c | 2 +-
> drivers/scsi/qla1280.c | 2 +-
> drivers/scsi/qla2xxx/qla_mid.c | 2 +-
> drivers/scsi/qla2xxx/qla_os.c | 2 +-
> drivers/scsi/qlogicfas.c | 2 +-
> drivers/scsi/qlogicpti.c | 2 +-
> drivers/scsi/scsi_debug.c | 2 +-
> drivers/scsi/sgiwd93.c | 2 +-
> drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
> drivers/scsi/snic/snic_main.c | 2 +-
> drivers/scsi/stex.c | 2 +-
> drivers/scsi/storvsc_drv.c | 2 +-
> drivers/scsi/sun3_scsi.c | 2 +-
> drivers/scsi/sun3x_esp.c | 2 +-
> drivers/scsi/sun_esp.c | 2 +-
> drivers/scsi/sym53c8xx_2/sym_glue.c | 2 +-
> drivers/scsi/virtio_scsi.c | 2 +-
> drivers/scsi/vmw_pvscsi.c | 2 +-
> drivers/scsi/wd719x.c | 2 +-
> drivers/scsi/xen-scsifront.c | 2 +-
> drivers/scsi/zorro_esp.c | 2 +-
> include/scsi/libfc.h | 2 +-
> include/scsi/scsi_host.h | 3 ++-
> 97 files changed, 107 insertions(+), 103 deletions(-)
>
Quite a lot of churn for such a (relatively) simple change.
I think it might be better to introduce a new function
(scsi_host_alloc_node() ?) with the additional parameter,
and make scsi_host_alloc() a wrapper around that.
That will reduce the size of this patch immensely.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.com +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply
* Re: [PATCH v3 1/4] scsi: scan: allocate sdev and starget on the NUMA node of the host adapter
From: Hannes Reinecke @ 2026-06-10 6:00 UTC (permalink / raw)
To: Sumit Saxena, Martin K . Petersen, Jens Axboe
Cc: James E . J . Bottomley, linux-scsi, linux-block, Adam Radford,
Khalid Aziz, Adaptec OEM Raid Solutions, Matthew Wilcox,
Juergen E . Fischer, Russell King, linux-arm-kernel, Finn Thain,
Michael Schmitz, Anil Gurumurthy, Sudarsana Kalluru,
Oliver Neukum, Ali Akcaagac, Jamie Lenehan, Ram Vegesna,
target-devel, Bradley Grove, Satish Kharat, Sesidhar Baddela,
Karan Tilak Kumar, Yihang Li, Don Brace, storagedev,
HighPoint Linux Team, Tyrel Datwyler, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, linuxppc-dev,
Brian King, Lee Duncan, Chris Leech, Mike Christie, open-iscsi,
Justin Tee, Paul Ely, Kashyap Desai, Shivasharan S,
Chandrakanth Patil, megaraidlinux.pdl, Sathya Prakash Veerichetty,
Sreekanth Reddy, mpi3mr-linuxdrv.pdl, Suganath Prabu Subramani,
Ranjan Kumar, MPT-FusionLinux.pdl, Daniel Palmer, GOTO Masanori,
YOKOTA Hiroshi, Jack Wang, Geoff Levand, Michael Reed,
Nilesh Javali, GR-QLogic-Storage-Upstream, Narsimhulu Musini,
K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
linux-hyperv, Michael S . Tsirkin, Jason Wang, Paolo Bonzini,
Stefan Hajnoczi, Eugenio Perez, virtualization, Vishal Bhakta,
bcm-kernel-feedback-list, Juergen Gross, Stefano Stabellini,
Oleksandr Tyshchenko, xen-devel, James Rizzo
In-Reply-To: <20260609121806.2121755-2-sumit.saxena@broadcom.com>
On 6/9/26 14:18, Sumit Saxena wrote:
> From: James Rizzo <james.rizzo@broadcom.com>
>
> When a host adapter is attached to a specific NUMA node, allocating
> scsi_device and scsi_target via kzalloc() may place them on a remote
> node. All hot-path I/O accesses to these structures then cross the NUMA
> interconnect, adding latency and consuming inter-node bandwidth.
>
> Use kzalloc_node() with dev_to_node(shost->dma_dev) so allocations land
> on the same node as the HBA, reducing cross-node traffic and improving
> I/O performance on NUMA systems.
>
> Signed-off-by: James Rizzo <james.rizzo@broadcom.com>
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> ---
> drivers/scsi/scsi_scan.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.com +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply
* Re: [PATCH v3] i2c: virtio: retain xfer with kref to fix UAF on interrupted wait
From: Viresh Kumar @ 2026-06-10 6:03 UTC (permalink / raw)
To: Gavin Li
Cc: linux-i2c, Chen, Jian Jun, andi.shyti, virtualization,
Wolfram Sang, Michael S. Tsirkin, Jason Wang
In-Reply-To: <20260609134358.36583-1-gavin.li@samsara.com>
Ccing I2C/Virtio maintainers.
On 09-06-26, 09:43, Gavin Li wrote:
> Additionally, force usage of a bounce buffer even if the i2c_msg buf is
> DMA-safe, since the buffer passed to the virtqueue must remain valid
> even if the transfer is interrupted. Remove usage of
> i2c_get_dma_safe_msg_buf() since it may pass through msg->buf directly.
> This results in an extra allocation+copy when I2C_M_DMA_SAFE is used.
Please always create separate patches for such changes. It would be better to
not fix multiple issues in the same patch. That would also allow for a clean
revert of the change, just in case.
> @@ -82,9 +116,16 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> sgs[outcnt++] = &out_hdr;
>
> if (msgs[i].len) {
> - reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
> + /*
> + * Even if msg->flags has I2C_M_DMA_SAFE set, a bounce
> + * buffer is required because the transfer may be
> + * interrupted, after which msg->buf is no longer valid.
> + */
> + reqs[i].buf = kzalloc(msgs[i].len, GFP_KERNEL);
> if (!reqs[i].buf)
> break;
> + if (!(msgs[i].flags & I2C_M_RD))
> + memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
>
> sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
I don't think this is acceptable, bringing the performance down because the
remote device may misbehave.
Maybe its time for I2C and Virtio maintainers to provide some feedback here.
--
viresh
^ permalink raw reply
* Re: [PATCH v3 3/4] block: drop shared-tag fairness throttling
From: Christoph Hellwig @ 2026-06-10 6:14 UTC (permalink / raw)
To: Sumit Saxena
Cc: Martin K . Petersen, Jens Axboe, James E . J . Bottomley,
linux-scsi, linux-block, Adam Radford, Khalid Aziz,
Adaptec OEM Raid Solutions, Matthew Wilcox, Hannes Reinecke,
Juergen E . Fischer, Russell King, linux-arm-kernel, Finn Thain,
Michael Schmitz, Anil Gurumurthy, Sudarsana Kalluru,
Oliver Neukum, Ali Akcaagac, Jamie Lenehan, Ram Vegesna,
target-devel, Bradley Grove, Satish Kharat, Sesidhar Baddela,
Karan Tilak Kumar, Yihang Li, Don Brace, storagedev,
HighPoint Linux Team, Tyrel Datwyler, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, linuxppc-dev,
Brian King, Lee Duncan, Chris Leech, Mike Christie, open-iscsi,
Justin Tee, Paul Ely, Kashyap Desai, Shivasharan S,
Chandrakanth Patil, megaraidlinux.pdl, Sathya Prakash Veerichetty,
Sreekanth Reddy, mpi3mr-linuxdrv.pdl, Suganath Prabu Subramani,
Ranjan Kumar, MPT-FusionLinux.pdl, Daniel Palmer, GOTO Masanori,
YOKOTA Hiroshi, Jack Wang, Geoff Levand, Michael Reed,
Nilesh Javali, GR-QLogic-Storage-Upstream, Narsimhulu Musini,
K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
linux-hyperv, Michael S . Tsirkin, Jason Wang, Paolo Bonzini,
Stefan Hajnoczi, Eugenio Perez, virtualization, Vishal Bhakta,
bcm-kernel-feedback-list, Juergen Gross, Stefano Stabellini,
Oleksandr Tyshchenko, xen-devel, Bart Van Assche
In-Reply-To: <20260609121806.2121755-4-sumit.saxena@broadcom.com>
Just dropping the fairness was rejected before and there is no
explanation here on why any of that has changed.
On Tue, Jun 09, 2026 at 05:48:02PM +0530, Sumit Saxena wrote:
> From: Bart Van Assche <bvanassche@acm.org>
>
> Original patch [1] by Bart Van Assche; this version is rebased onto the
> current tree. In testing it improves IOPS by roughly 16-18% by removing
> the fair-sharing throttle on shared tag queues.
>
> This patch removes the following code and structure members:
> - The function hctx_may_queue().
> - blk_mq_hw_ctx.nr_active and request_queue.nr_active_requests_shared_tags
> and also all the code that modifies these two member variables.
.. and besides that, this commit message is still entirely useless
as it doesn't explain any of the thoughts of why this change is safe
and desirable. While the mechanics above are totally obvious from
the code change itself.
^ permalink raw reply
* Re: [PATCH v3 3/4] block: drop shared-tag fairness throttling
From: Hannes Reinecke @ 2026-06-10 6:18 UTC (permalink / raw)
To: Sumit Saxena, Martin K . Petersen, Jens Axboe
Cc: James E . J . Bottomley, linux-scsi, linux-block, Adam Radford,
Khalid Aziz, Adaptec OEM Raid Solutions, Matthew Wilcox,
Hannes Reinecke, Juergen E . Fischer, Russell King,
linux-arm-kernel, Finn Thain, Michael Schmitz, Anil Gurumurthy,
Sudarsana Kalluru, Oliver Neukum, Ali Akcaagac, Jamie Lenehan,
Ram Vegesna, target-devel, Bradley Grove, Satish Kharat,
Sesidhar Baddela, Karan Tilak Kumar, Yihang Li, Don Brace,
storagedev, HighPoint Linux Team, Tyrel Datwyler,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, linuxppc-dev, Brian King, Lee Duncan,
Chris Leech, Mike Christie, open-iscsi, Justin Tee, Paul Ely,
Kashyap Desai, Shivasharan S, Chandrakanth Patil,
megaraidlinux.pdl, Sathya Prakash Veerichetty, Sreekanth Reddy,
mpi3mr-linuxdrv.pdl, Suganath Prabu Subramani, Ranjan Kumar,
MPT-FusionLinux.pdl, Daniel Palmer, GOTO Masanori, YOKOTA Hiroshi,
Jack Wang, Geoff Levand, Michael Reed, Nilesh Javali,
GR-QLogic-Storage-Upstream, Narsimhulu Musini, K . Y . Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, linux-hyperv,
Michael S . Tsirkin, Jason Wang, Paolo Bonzini, Stefan Hajnoczi,
Eugenio Perez, virtualization, Vishal Bhakta,
bcm-kernel-feedback-list, Juergen Gross, Stefano Stabellini,
Oleksandr Tyshchenko, xen-devel, Bart Van Assche
In-Reply-To: <20260609121806.2121755-4-sumit.saxena@broadcom.com>
On 6/9/26 14:18, Sumit Saxena wrote:
> From: Bart Van Assche <bvanassche@acm.org>
>
> Original patch [1] by Bart Van Assche; this version is rebased onto the
> current tree. In testing it improves IOPS by roughly 16-18% by removing
> the fair-sharing throttle on shared tag queues.
>
> This patch removes the following code and structure members:
> - The function hctx_may_queue().
> - blk_mq_hw_ctx.nr_active and request_queue.nr_active_requests_shared_tags
> and also all the code that modifies these two member variables.
>
> [1]: https://lore.kernel.org/linux-block/20240529213921.3166462-1-bvanassche@acm.org/
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> ---
> block/blk-core.c | 2 -
> block/blk-mq-debugfs.c | 22 ++++++++-
> block/blk-mq-tag.c | 4 --
> block/blk-mq.c | 17 +------
> block/blk-mq.h | 100 -----------------------------------------
> include/linux/blk-mq.h | 6 ---
> include/linux/blkdev.h | 2 -
> 7 files changed, 22 insertions(+), 131 deletions(-)
>
What tests did you perform?
I'm pretty sure you see an improvement when having just a few drives,
but what about having a lot of them (ie tens of drives)?
The whole point of this was to increase fairness between drives, so
of course removing it will make an individual drive going faster ...
Maybe it's an idea to move the fairness algorithm into an I/O scheduler;
that way we can keep the original behaviour yet get the performance
increase if people want it.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply
* Re: [PATCH v3 4/4] scsi: use percpu counters for iostat counters in struct scsi_device
From: Hannes Reinecke @ 2026-06-10 6:21 UTC (permalink / raw)
To: Sumit Saxena, Martin K . Petersen, Jens Axboe
Cc: James E . J . Bottomley, linux-scsi, linux-block, Adam Radford,
Khalid Aziz, Adaptec OEM Raid Solutions, Matthew Wilcox,
Hannes Reinecke, Juergen E . Fischer, Russell King,
linux-arm-kernel, Finn Thain, Michael Schmitz, Anil Gurumurthy,
Sudarsana Kalluru, Oliver Neukum, Ali Akcaagac, Jamie Lenehan,
Ram Vegesna, target-devel, Bradley Grove, Satish Kharat,
Sesidhar Baddela, Karan Tilak Kumar, Yihang Li, Don Brace,
storagedev, HighPoint Linux Team, Tyrel Datwyler,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, linuxppc-dev, Brian King, Lee Duncan,
Chris Leech, Mike Christie, open-iscsi, Justin Tee, Paul Ely,
Kashyap Desai, Shivasharan S, Chandrakanth Patil,
megaraidlinux.pdl, Sathya Prakash Veerichetty, Sreekanth Reddy,
mpi3mr-linuxdrv.pdl, Suganath Prabu Subramani, Ranjan Kumar,
MPT-FusionLinux.pdl, Daniel Palmer, GOTO Masanori, YOKOTA Hiroshi,
Jack Wang, Geoff Levand, Michael Reed, Nilesh Javali,
GR-QLogic-Storage-Upstream, Narsimhulu Musini, K . Y . Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, linux-hyperv,
Michael S . Tsirkin, Jason Wang, Paolo Bonzini, Stefan Hajnoczi,
Eugenio Perez, virtualization, Vishal Bhakta,
bcm-kernel-feedback-list, Juergen Gross, Stefano Stabellini,
Oleksandr Tyshchenko, xen-devel, John Garry
In-Reply-To: <20260609121806.2121755-5-sumit.saxena@broadcom.com>
On 6/9/26 14:18, Sumit Saxena wrote:
> iorequest_cnt and iodone_cnt are updated on every command dispatch and
> completion, often from different CPUs on high queue depth workloads.
> Using adjacent atomic_t fields causes cache line contention between the
> submission and completion paths.
>
> Extend the same treatment to ioerr_cnt and iotmo_cnt so all four iostat
> counters in struct scsi_device use struct percpu_counter.
>
> Suggested-by: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> ---
> drivers/scsi/scsi_error.c | 4 ++--
> drivers/scsi/scsi_lib.c | 10 +++++-----
> drivers/scsi/scsi_scan.c | 8 ++++++++
> drivers/scsi/scsi_sysfs.c | 23 ++++++++++++++---------
> drivers/scsi/sd.c | 2 +-
> include/scsi/scsi_device.h | 9 +++++----
> 6 files changed, 35 insertions(+), 21 deletions(-)
>
Good idea.
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply
* Re: [PATCH v3] i2c: virtio: retain xfer with kref to fix UAF on interrupted wait
From: Michael S. Tsirkin @ 2026-06-10 6:50 UTC (permalink / raw)
To: Gavin Li
Cc: linux-i2c, viresh.kumar, Chen, Jian Jun, andi.shyti,
virtualization
In-Reply-To: <20260609134358.36583-1-gavin.li@samsara.com>
On Tue, Jun 09, 2026 at 09:43:58AM -0400, Gavin Li wrote:
> commit a663b3c47ab1 ("i2c: virtio: Avoid hang by using interruptible
> completion wait") switched virtio_i2c_complete_reqs() to
> wait_for_completion_interruptible() so a stuck device cannot hang a
> task forever. That left a use-after-free: if the wait returns early on
> a signal, virtio_i2c_xfer() frees reqs and DMA bounce buffers while the
> device may still hold virtqueue tokens pointing at &reqs[i] and DMA
> into read buffers. When those requests complete later,
> virtio_i2c_msg_done() calls complete() on freed memory.
>
> Waiting uninterruptibly for every completion before freeing avoids the
> UAF but can hang the caller indefinitely if the virtio side never
> completes the request. The virtio spec provides no way to cancel an
> in-flight transfer, so that is not an acceptable tradeoff.
except for a queue reset, or a device reset if that's not
available.
> This commit manages the freeing of the xfer allocations via kref, and
> ensures that each in-flight request holds a reference. This fixes the
> use-after-free by ensuring that the virtio device has a valid location
> to write to until the request completes. This will cause a memory leak
> in cases where the device hangs, but that is much preferable to memory
> corruption.
>
> Additionally, force usage of a bounce buffer even if the i2c_msg buf is
> DMA-safe, since the buffer passed to the virtqueue must remain valid
> even if the transfer is interrupted. Remove usage of
> i2c_get_dma_safe_msg_buf() since it may pass through msg->buf directly.
> This results in an extra allocation+copy when I2C_M_DMA_SAFE is used.
>
> Signed-off-by: Gavin Li <gavin.li@samsara.com>
It's possible that copying unconditionally is the best you could do.
Some vague ideas below but it's possible they are not practical.
> ---
pls start a new thread with each version, and include a changelog there.
> drivers/i2c/busses/i2c-virtio.c | 110 ++++++++++++++++++++++++--------
> 1 file changed, 83 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> index 5da6fef92bec3..7ee96b08f6685 100644
> --- a/drivers/i2c/busses/i2c-virtio.c
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -13,6 +13,7 @@
> #include <linux/err.h>
> #include <linux/i2c.h>
> #include <linux/kernel.h>
> +#include <linux/kref.h>
> #include <linux/module.h>
> #include <linux/virtio.h>
> #include <linux/virtio_ids.h>
> @@ -31,39 +32,72 @@ struct virtio_i2c {
> struct virtqueue *vq;
> };
>
> +struct virtio_i2c_xfer;
> +
> /**
> * struct virtio_i2c_req - the virtio I2C request structure
> + * @xfer: owning transfer
> * @completion: completion of virtio I2C message
> * @out_hdr: the OUT header of the virtio I2C message
> - * @buf: the buffer into which data is read, or from which it's written
> + * @buf: bounce buffer for i2c_msg.buf, set to NULL upon free
> * @in_hdr: the IN header of the virtio I2C message
> */
> struct virtio_i2c_req {
> + struct virtio_i2c_xfer *xfer;
> struct completion completion;
> struct virtio_i2c_out_hdr out_hdr ____cacheline_aligned;
> uint8_t *buf ____cacheline_aligned;
> struct virtio_i2c_in_hdr in_hdr ____cacheline_aligned;
> };
>
> +/**
> + * struct virtio_i2c_xfer - a queued I2C transfer
> + * @ref: one ref for the caller, plus one per in-flight virtqueue request
> + * @num: number of messages
> + * @reqs: the virtio I2C requests
> + */
> +struct virtio_i2c_xfer {
> + struct kref ref;
> + int num;
> + struct virtio_i2c_req reqs[];
> +};
> +
> +static void virtio_i2c_xfer_release(struct kref *ref)
> +{
> + struct virtio_i2c_xfer *xfer = container_of(ref, struct virtio_i2c_xfer, ref);
> + int i;
> +
> + for (i = 0; i < xfer->num; i++) {
> + struct virtio_i2c_req *req = &xfer->reqs[i];
> + kfree(req->buf);
> + }
> +
> + kfree(xfer);
> +}
> +
> static void virtio_i2c_msg_done(struct virtqueue *vq)
> {
> struct virtio_i2c_req *req;
> unsigned int len;
>
> - while ((req = virtqueue_get_buf(vq, &len)))
> + while ((req = virtqueue_get_buf(vq, &len))) {
> complete(&req->completion);
> + kref_put(&req->xfer->ref, virtio_i2c_xfer_release);
> + }
> }
>
> -static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> - struct virtio_i2c_req *reqs,
> +static int virtio_i2c_prepare_xfer(struct virtqueue *vq,
> + struct virtio_i2c_xfer *xfer,
> struct i2c_msg *msgs, int num)
> {
> struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + struct virtio_i2c_req *reqs = xfer->reqs;
> int i;
>
> for (i = 0; i < num; i++) {
> int outcnt = 0, incnt = 0;
>
> + reqs[i].xfer = xfer;
> init_completion(&reqs[i].completion);
>
> /*
> @@ -82,9 +116,16 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> sgs[outcnt++] = &out_hdr;
>
> if (msgs[i].len) {
> - reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
> + /*
> + * Even if msg->flags has I2C_M_DMA_SAFE set, a bounce
> + * buffer is required because the transfer may be
> + * interrupted, after which msg->buf is no longer valid.
> + */
> + reqs[i].buf = kzalloc(msgs[i].len, GFP_KERNEL);
is len always reasonably small?
> if (!reqs[i].buf)
> break;
> + if (!(msgs[i].flags & I2C_M_RD))
> + memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
>
> sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
>
maybe there's some way to hang on to the original memory?
> @@ -97,38 +138,51 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> sgs[outcnt + incnt++] = &in_hdr;
>
> + /* This reference is released in virtio_i2c_msg_done(). */
> + kref_get(&xfer->ref);
> +
> if (virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL)) {
> - i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
> + kref_put(&xfer->ref, virtio_i2c_xfer_release);
> +
> + kfree(reqs[i].buf);
> + reqs[i].buf = NULL; /* prevent free by virtio_i2c_xfer_release */
> +
> break;
> }
> }
>
> + xfer->num = i;
> return i;
> }
>
> -static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> - struct virtio_i2c_req *reqs,
> - struct i2c_msg *msgs, int num)
> +static int virtio_i2c_complete_xfer(struct virtio_i2c_xfer *xfer,
> + struct i2c_msg *msgs)
> {
> - bool failed = false;
> - int i, j = 0;
> + struct virtio_i2c_req *reqs = xfer->reqs;
> + int i, fail_index = -1;
>
> - for (i = 0; i < num; i++) {
> + for (i = 0; i < xfer->num; i++) {
> struct virtio_i2c_req *req = &reqs[i];
> + struct i2c_msg *msg = &msgs[i];
>
> - if (!failed) {
> - if (wait_for_completion_interruptible(&req->completion))
> - failed = true;
> - else if (req->in_hdr.status != VIRTIO_I2C_MSG_OK)
> - failed = true;
> - else
> - j++;
> + if (wait_for_completion_interruptible(&req->completion))
> + return -EINTR;
> +
> + if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
> + /* Don't break yet. Try to wait until all requests complete. */
> + if (fail_index < 0)
> + fail_index = i;
> }
>
> - i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
> + if (fail_index < 0 && (msg->flags & I2C_M_RD))
> + memcpy(msg->buf, req->buf, msg->len);
> +
> + kfree(req->buf);
> + req->buf = NULL; /* prevent free by virtio_i2c_xfer_release */
> }
>
> - return j;
> + /* Return number of successful transactions */
> + return fail_index >= 0 ? fail_index : xfer->num;
> }
>
> static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> @@ -136,14 +190,16 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> {
> struct virtio_i2c *vi = i2c_get_adapdata(adap);
> struct virtqueue *vq = vi->vq;
> - struct virtio_i2c_req *reqs;
> + struct virtio_i2c_xfer *xfer;
> int count;
>
> - reqs = kzalloc_objs(*reqs, num);
> - if (!reqs)
> + xfer = kzalloc(struct_size(xfer, reqs, num), GFP_KERNEL);
> + if (!xfer)
> return -ENOMEM;
>
> - count = virtio_i2c_prepare_reqs(vq, reqs, msgs, num);
> + kref_init(&xfer->ref);
> +
> + count = virtio_i2c_prepare_xfer(vq, xfer, msgs, num);
> if (!count)
> goto err_free;
>
> @@ -157,10 +213,10 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> */
> virtqueue_kick(vq);
>
> - count = virtio_i2c_complete_reqs(vq, reqs, msgs, count);
> + count = virtio_i2c_complete_xfer(xfer, msgs);
>
> err_free:
> - kfree(reqs);
> + kref_put(&xfer->ref, virtio_i2c_xfer_release);
> return count;
> }
>
> --
> 2.54.0
>
^ permalink raw reply
* Re: [PATCH] [v3] vduse: fix compat handling for VDUSE_IOTLB_GET_FD/VDUSE_VQ_GET_INFO
From: Arnd Bergmann @ 2026-06-10 6:54 UTC (permalink / raw)
To: Michael S. Tsirkin, Arnd Bergmann
Cc: Jason Wang, Xie Yongji, Eugenio Pérez, Xuan Zhuo,
Marco Crivellari, Anders Roxell, virtualization, linux-kernel
In-Reply-To: <20260610012421-mutt-send-email-mst@kernel.org>
On Wed, Jun 10, 2026, at 07:26, Michael S. Tsirkin wrote:
> On Fri, Feb 13, 2026 at 04:40:46PM +0100, Arnd Bergmann wrote:
>> + };
>> + __u8 ready;
>> + __u8 padding[3];
>> +} __uapi_arch_align;
>
> what is this __uapi_arch_align supposed to be doing?
>
> It compiles by luck because gcc thinks it's a global variable.
Sorry, that should not have been part of this patch, it is
part of the series of changes I was testing when I noticed
the problem here, but that other series is unfortunately
still not ready for merging.
The background here is that I'm adding -Werror=padded to
the UAPI checks in usr/include/Makefile, in order to find
any instances of data structures with implied padding,
and then instead add explicit padding using architecture
specific macros. In vduse_vq_info, this includes a final
32 bit of padding on architectures with naturally
aligned __u64 but no padding when __u64 has a smaller
alignment.
The __uapi_arch_align macro in turn is what I add to
structures with optional padding, in order to be able
to reduce the alignment of a structure when building the
kernel with a higher default alignment than userspace
(e.g. on m68k with -malign-int).
Removing the __uapi_arch_align is the correct fix for the
moment. Let me know if you would like me to send a
replacement patch without it, a fixup patch on top, or if
you will fix it up yourself.
Arnd
^ permalink raw reply
* Re: [PATCH] [v3] vduse: fix compat handling for VDUSE_IOTLB_GET_FD/VDUSE_VQ_GET_INFO
From: Michael S. Tsirkin @ 2026-06-10 6:58 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Arnd Bergmann, Jason Wang, Xie Yongji, Eugenio Pérez,
Xuan Zhuo, Marco Crivellari, Anders Roxell, virtualization,
linux-kernel
In-Reply-To: <0134edd1-de51-46b3-9ea8-bdbcf48b878f@app.fastmail.com>
On Wed, Jun 10, 2026 at 08:54:59AM +0200, Arnd Bergmann wrote:
> On Wed, Jun 10, 2026, at 07:26, Michael S. Tsirkin wrote:
> > On Fri, Feb 13, 2026 at 04:40:46PM +0100, Arnd Bergmann wrote:
> >> + };
> >> + __u8 ready;
> >> + __u8 padding[3];
> >> +} __uapi_arch_align;
> >
> > what is this __uapi_arch_align supposed to be doing?
> >
> > It compiles by luck because gcc thinks it's a global variable.
>
> Sorry, that should not have been part of this patch, it is
> part of the series of changes I was testing when I noticed
> the problem here, but that other series is unfortunately
> still not ready for merging.
>
> The background here is that I'm adding -Werror=padded to
> the UAPI checks in usr/include/Makefile, in order to find
> any instances of data structures with implied padding,
> and then instead add explicit padding using architecture
> specific macros. In vduse_vq_info, this includes a final
> 32 bit of padding on architectures with naturally
> aligned __u64 but no padding when __u64 has a smaller
> alignment.
>
> The __uapi_arch_align macro in turn is what I add to
> structures with optional padding, in order to be able
> to reduce the alignment of a structure when building the
> kernel with a higher default alignment than userspace
> (e.g. on m68k with -malign-int).
>
> Removing the __uapi_arch_align is the correct fix for the
> moment. Let me know if you would like me to send a
> replacement patch without it, a fixup patch on top, or if
> you will fix it up yourself.
>
> Arnd
I fixed it, thanks.
^ permalink raw reply
* Re: [PATCH RESEND] virtio_console: read size from config space during device init
From: Michael S. Tsirkin @ 2026-06-10 7:04 UTC (permalink / raw)
To: Filip Hejsek
Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Rusty Russell,
virtualization, linux-kernel
In-Reply-To: <20260223-virtio-console-fix-v1-1-0cf08303b428@gmail.com>
On Mon, Feb 23, 2026 at 06:37:02PM +0100, Filip Hejsek wrote:
> Previously, the size was only read upon receiving the config interrupt.
> This interrupt is sent when the size changes. However, we also need to
> read the initial size.
>
> Also make sure to only read the size from config if F_SIZE is enabled.
>
> Fixes: 9778829cffd4 ("virtio: console: Store each console's size in the console structure")
> Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
> ---
> This is a resend of [1], which hasn't received any response.
>
> I found this bug while developing patches for QEMU that add virtio
> console resize support. If you want to test this, you can get my QEMU
> patches from [2]. You will need to disable multiport
> using `-device virtio-serial,max_ports=1`.
>
> [1]: https://lore.kernel.org/all/20251224-virtio-console-fix-v1-1-69d0349692dc@gmail.com/
> [2]: https://lore.kernel.org/all/20250921-console-resize-v5-0-89e3c6727060@gmail.com/
>
> I'll also repeat my questions from the previous submission here. These are
> things that confused me when I was trying to understand the surrounding code,
> but should in no way prevent merging this patch.
>
> - Why does use_multiport use __virtio_test_bit instead of
> virtio_has_feature?
>
> - The VIRTIO_CONSOLE_RESIZE handler sets irq_requested to 1, which I
> think makes no sense?
> ---
> drivers/char/virtio_console.c | 52 ++++++++++++++++++++++++++-----------------
> 1 file changed, 31 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 088182e54d..c355f6d392 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1771,32 +1771,40 @@ static void config_intr(struct virtio_device *vdev)
> schedule_work(&portdev->config_work);
> }
>
> -static void config_work_handler(struct work_struct *work)
> +static void update_size_from_config(struct ports_device *portdev)
> {
> - struct ports_device *portdev;
> + struct virtio_device *vdev;
> + struct port *port;
> + u16 rows, cols;
>
> - portdev = container_of(work, struct ports_device, config_work);
> - if (!use_multiport(portdev)) {
> - struct virtio_device *vdev;
> - struct port *port;
> - u16 rows, cols;
> + vdev = portdev->vdev;
>
> - vdev = portdev->vdev;
> - virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> - virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> + /*
> + * We'll use this way of resizing only for legacy support.
> + * For multiport devices, use control messages to indicate
> + * console size changes so that it can be done per-port.
> + *
> + * Don't test F_SIZE at all if we're rproc: not a valid feature.
> + */
> + if (is_rproc_serial(vdev) ||
Wait a second. Why is there this rproc test here?
Was not in the original code and commit log says nothing about it.
> + use_multiport(portdev) ||
> + !virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> + return;
>
> - port = find_port_by_id(portdev, 0);
> - set_console_size(port, rows, cols);
> + virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> + virtio_cread(vdev, struct virtio_console_config, rows, &rows);
>
> - /*
> - * We'll use this way of resizing only for legacy
> - * support. For newer userspace
> - * (VIRTIO_CONSOLE_F_MULTPORT+), use control messages
> - * to indicate console size changes so that it can be
> - * done per-port.
> - */
> - resize_console(port);
> - }
> + port = find_port_by_id(portdev, 0);
> + set_console_size(port, rows, cols);
> + resize_console(port);
> +}
> +
> +static void config_work_handler(struct work_struct *work)
> +{
> + struct ports_device *portdev;
> +
> + portdev = container_of(work, struct ports_device, config_work);
> + update_size_from_config(portdev);
> }
>
> static int init_vqs(struct ports_device *portdev)
> @@ -2054,6 +2062,8 @@ static int virtcons_probe(struct virtio_device *vdev)
> __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
> VIRTIO_CONSOLE_DEVICE_READY, 1);
>
> + update_size_from_config(portdev);
> +
> return 0;
>
> free_chrdev:
>
> ---
> base-commit: b927546677c876e26eba308550207c2ddf812a43
> change-id: 20251224-virtio-console-fix-3d46980ef569
>
> Best regards,
> --
> Filip Hejsek <filip.hejsek@gmail.com>
^ permalink raw reply
* Re: [PATCH v15] can: virtio: Add virtio CAN driver
From: Michael S. Tsirkin @ 2026-06-10 7:08 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: Harald Mommer, Marc Kleine-Budde, Vincent Mailhol, Jason Wang,
Xuan Zhuo, Eugenio Pérez, linux-can, virtualization,
Mikhail Golubev-Ciuchea, Stefano Garzarella, francesco
In-Reply-To: <ahXNb+KzuHYbS24+@fedora>
On Tue, May 26, 2026 at 06:42:23PM +0200, Matias Ezequiel Vara Larsen wrote:
> Add virtio CAN driver based on Virtio 1.4 specification (see
> https://github.com/oasis-tcs/virtio-spec/tree/virtio-1.4). The driver
> implements a complete CAN bus interface over Virtio transport,
> supporting both CAN Classic and CAN-FD Ids. In term of frames, it
> supports classic and CAN FD. RTR frames are only supported with classic
> CAN.
>
> Usage:
> - "ip link set up can0" - start controller
> - "ip link set down can0" - stop controller
> - "candump can0" - receive frames
> - "cansend can0 123#DEADBEEF" - send frames
>
> Signed-off-by: Harald Mommer <harald.mommer@oss.qualcomm.com>
> Co-developed-by: Harald Mommer <harald.mommer@oss.qualcomm.com>
> Signed-off-by: Mikhail Golubev-Ciuchea <mikhail.golubev-ciuchea@oss.qualcomm.com>
> Co-developed-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Damir Shaikhutdinov <Damir.Shaikhutdinov@opensynergy.com>
> Reviewed-by: Francesco Valla <francesco@valla.it>
> Tested-by: Francesco Valla <francesco@valla.it>
> Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
It's in next now. Marc, Vincent, should I send it upstream in the next pull?
> ---
> V15:
> - Drop unchecked can_put_echo_skb() return value and dead error check in
> virtio_can_freeze() since virtio_can_close() always returns zero.
> - Fix two use-after-free bugs: in virtio_can_start_xmit(), returning
> NETDEV_TX_BUSY after virtqueue_add_sgs() fails is wrong because
> can_put_echo_skb() already consumed the skb; and in the freeze/restore/remove
> path, stale vq pointers left by a failed restore() are dereferenced by a
> subsequent remove().
>
> V14:
> - set length before memcpy for field defined with __counted_by_le()
> - move napi_enable() to open() and napi_disable() to close() thus following
> pattern in other CAN devices
> - add flag to prevent remove() to hang if restore() fails
>
> V13:
> - kick device only when some buffers have been added
> - add virtio_can_del_vq() when virtio_can_start() fails
> - move napi_disable() at the end of virtio_can_freeze()
> - move napi_enable() at beginning of virtio_can_restore()
> - let virtio_can_start() sets CAN_STATE_ERROR_ACTIVE
> - in freeze(), if can_stop() fails, restore the state of the device
> - in restore(), if !netif_running, set CAN_STATE_STOPPED
>
> V12:
> - check return value in virtio_can_start()
> - use devm_krealloc() in virtio_can_restore() to re-allocate rpkt
> - remove access to cf->len and replace it with len
> - use __counted_by_le()
> - free tx buffers during virtio_can_del_vq()
> - add reinit_completion() for ctrl msgs
>
> V11:
> * Set CAN_VIRTIO_CAN config before CAN_XILINXCAN
> * Use GFP_ATOMIC in virtio_can_alloc_tx_idx()
> * Use open_candev() and set bittiming.bitrate to CAN_BITRATE_UNKNOWN and
> data_bittiming.bitrate to CAN_BITRATE_UNKNOWN
> * Fix leak of skb by adding kfree_skb(skb)
> * Fix out-of-bounds in virtio_can_populate_rx_vq()
> * Initialize `ret`
> * Use virtio_reset_device()
> * Add napi_disable() in virtio_can_remove()
> * Propagate error in virtio_can_start() and virtio_can_stop()
>
> V10:
> * Follow Reverse Christmas Tree convention
>
> V9:
> * Remove unnecessary comments
> * Update maintainer list
>
> V8:
> * Address nits
>
> V7:
> * Address nits
> * Remove unnecessary comments
> * Remove io_callbacks[]
> * Use guard() syntax
> * Remove kicking for each inbuf
> * replace sdu_len with rpkt_len
> * Use devm_kzalloc()
> * Use scoped_guard() to protect virtqueue_add_sgs() and virtqueue_kicks() for
> tx queue
> * Tested with vhost-device-can
> (see https://github.com/rust-vmm/vhost-device/tree/main/vhost-device-can) and
> Qemu (942b0d3) with [1]. A reviewer observed that the device stops to work
> after flooding from host. This issue is still present.
>
> [1]
> https://lore.kernel.org/qemu-devel/20251031155617.1223248-1-mvaralar@redhat.com/
>
> V6:
> * Address nits
> * Check for error during register_virtio_can()
> * Remove virtio_device_ready()
> * Allocate virtio_can_rx rpkt[] at probe
> * Define virtio_can_control struct
> * Return VIRTIO_CAN_RESULT_NOT_OK after unlocking
> * Define sdu[] as a flex array for both tx and rx. For rx, use
> VIRTIO_CAN_F_CAN_FD to figure out the max len for sdu
> * Fix statistics in virtio_can_read_tx_queue() and
> how we indicate error to the user when getting
> VIRTIO_CAN_RESULT_NOT_OK
> * Fix syntax of virtio_find_vqs()
> * Drop tx_list
> * Fix values of VIRTIO_CAN_F_LATE_TX_ACK and VIRTIO_CAN_F_RTR_FRAMES
> * Tested with vhost-device-can
> (see
> https://github.com/rust-vmm/vhost-device/tree/main/vhost-device-can)
> and qemu (see
> https://github.com/virtualopensystems/qemu/tree/vhu-can-rfc)
>
> V5:
> * Re-base on top of linux-next (next-20240103)
> * Tested with https://github.com/OpenSynergy/qemu/tree/virtio-can-spec-rfc-v3
>
> RFC V4:
> * Apply reverse Christmas tree style
> * Add member *classic_dlc to RX and TX CAN frames
> * Fix race causing a NETDEV_TX_BUSY return
> ---
> MAINTAINERS | 9 +
> drivers/net/can/Kconfig | 12 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/virtio_can.c | 1022 +++++++++++++++++++++++++++++++
> include/uapi/linux/virtio_can.h | 78 +++
> 5 files changed, 1122 insertions(+)
> create mode 100644 drivers/net/can/virtio_can.c
> create mode 100644 include/uapi/linux/virtio_can.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 80cd3498c293..f295a904c93e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -27068,6 +27068,15 @@ F: drivers/scsi/virtio_scsi.c
> F: include/uapi/linux/virtio_blk.h
> F: include/uapi/linux/virtio_scsi.h
>
> +VIRTIO CAN DRIVER
> +M: "Harald Mommer" <harald.mommer@oss.qualcomm.com>
> +M: "Matias Ezequiel Vara Larsen" <mvaralar@redhat.com>
> +L: virtualization@lists.linux.dev
> +L: linux-can@vger.kernel.org
> +S: Maintained
> +F: drivers/net/can/virtio_can.c
> +F: include/uapi/linux/virtio_can.h
> +
> VIRTIO CONSOLE DRIVER
> M: Amit Shah <amit@kernel.org>
> L: virtualization@lists.linux.dev
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index d43d56694667..f33ae46d9766 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -209,6 +209,18 @@ config CAN_TI_HECC
> Driver for TI HECC (High End CAN Controller) module found on many
> TI devices. The device specifications are available from www.ti.com
>
> +config CAN_VIRTIO_CAN
> + depends on VIRTIO
> + tristate "Virtio CAN device support"
> + default n
> + help
> + Say Y here if you want to support for Virtio CAN.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called virtio-can.
> +
> + If unsure, say N.
> +
> config CAN_XILINXCAN
> tristate "Xilinx CAN"
> depends on ARCH_ZYNQ || ARM64 || MICROBLAZE || COMPILE_TEST
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 56138d8ddfd2..2ddea733ed5d 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/
> obj-$(CONFIG_CAN_SJA1000) += sja1000/
> obj-$(CONFIG_CAN_SUN4I) += sun4i_can.o
> obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
> +obj-$(CONFIG_CAN_VIRTIO_CAN) += virtio_can.o
> obj-$(CONFIG_CAN_XILINXCAN) += xilinx_can.o
>
> subdir-ccflags-$(CONFIG_CAN_DEBUG_DEVICES) += -DDEBUG
> diff --git a/drivers/net/can/virtio_can.c b/drivers/net/can/virtio_can.c
> new file mode 100644
> index 000000000000..f67d0bf09681
> --- /dev/null
> +++ b/drivers/net/can/virtio_can.c
> @@ -0,0 +1,1022 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * CAN bus driver for the Virtio CAN controller
> + *
> + * Copyright (C) 2021-2023 OpenSynergy GmbH
> + * Copyright Red Hat, Inc. 2025
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/idr.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/netdevice.h>
> +#include <linux/stddef.h>
> +#include <linux/can/dev.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ring.h>
> +#include <linux/virtio_can.h>
> +
> +/* CAN device queues */
> +#define VIRTIO_CAN_QUEUE_TX 0
> +#define VIRTIO_CAN_QUEUE_RX 1
> +#define VIRTIO_CAN_QUEUE_CONTROL 2
> +#define VIRTIO_CAN_QUEUE_COUNT 3
> +
> +#define CAN_KNOWN_FLAGS \
> + (VIRTIO_CAN_FLAGS_EXTENDED |\
> + VIRTIO_CAN_FLAGS_FD |\
> + VIRTIO_CAN_FLAGS_RTR)
> +
> +/* Max. number of in flight TX messages */
> +#define VIRTIO_CAN_ECHO_SKB_MAX 128
> +
> +struct virtio_can_tx {
> + unsigned int putidx;
> + struct virtio_can_tx_in tx_in;
> + /* Keep virtio_can_tx_out at the end of the structure due to flex array */
> + struct virtio_can_tx_out tx_out;
> +};
> +
> +struct virtio_can_control {
> + struct virtio_can_control_out cpkt_out;
> + struct virtio_can_control_in cpkt_in;
> +};
> +
> +/* virtio_can private data structure */
> +struct virtio_can_priv {
> + struct can_priv can; /* must be the first member */
> + /* NAPI for RX messages */
> + struct napi_struct napi;
> + /* NAPI for TX messages */
> + struct napi_struct napi_tx;
> + /* The network device we're associated with */
> + struct net_device *dev;
> + /* The virtio device we're associated with */
> + struct virtio_device *vdev;
> + /* The virtqueues */
> + struct virtqueue *vqs[VIRTIO_CAN_QUEUE_COUNT];
> + /* Lock for TX operations */
> + spinlock_t tx_lock;
> + /* Control queue lock */
> + struct mutex ctrl_lock;
> + /* Wait for control queue processing without polling */
> + struct completion ctrl_done;
> + /* Array of receive queue messages */
> + struct virtio_can_rx *rpkt;
> + struct virtio_can_control can_ctr_msg;
> + /* Data to get and maintain the putidx for local TX echo */
> + struct ida tx_putidx_ida;
> + /* In flight TX messages */
> + atomic_t tx_inflight;
> + /* Packet length */
> + int rpkt_len;
> + /* BusOff pending. Reset after successful indication to upper layer */
> + bool busoff_pending;
> + /* Tracks whether NAPI instances are currently enabled */
> + bool napi_active;
> +};
> +
> +static void virtqueue_napi_schedule(struct napi_struct *napi,
> + struct virtqueue *vq)
> +{
> + if (napi_schedule_prep(napi)) {
> + virtqueue_disable_cb(vq);
> + __napi_schedule(napi);
> + }
> +}
> +
> +static void virtqueue_napi_complete(struct napi_struct *napi,
> + struct virtqueue *vq, int processed)
> +{
> + int opaque;
> +
> + opaque = virtqueue_enable_cb_prepare(vq);
> + if (napi_complete_done(napi, processed)) {
> + if (unlikely(virtqueue_poll(vq, opaque)))
> + virtqueue_napi_schedule(napi, vq);
> + } else {
> + virtqueue_disable_cb(vq);
> + }
> +}
> +
> +static void virtio_can_free_candev(struct net_device *ndev)
> +{
> + struct virtio_can_priv *priv = netdev_priv(ndev);
> +
> + ida_destroy(&priv->tx_putidx_ida);
> + free_candev(ndev);
> +}
> +
> +static void virtio_can_napi_enable(struct virtio_can_priv *priv)
> +{
> + if (!priv->napi_active) {
> + napi_enable(&priv->napi);
> + napi_enable(&priv->napi_tx);
> + priv->napi_active = true;
> + }
> +}
> +
> +static void virtio_can_napi_disable(struct virtio_can_priv *priv)
> +{
> + if (priv->napi_active) {
> + napi_disable(&priv->napi_tx);
> + napi_disable(&priv->napi);
> + priv->napi_active = false;
> + }
> +}
> +
> +static int virtio_can_alloc_tx_idx(struct virtio_can_priv *priv)
> +{
> + int tx_idx;
> +
> + tx_idx = ida_alloc_max(&priv->tx_putidx_ida,
> + priv->can.echo_skb_max - 1, GFP_ATOMIC);
> + if (tx_idx >= 0)
> + atomic_inc(&priv->tx_inflight);
> +
> + return tx_idx;
> +}
> +
> +static void virtio_can_free_tx_idx(struct virtio_can_priv *priv,
> + unsigned int idx)
> +{
> + ida_free(&priv->tx_putidx_ida, idx);
> + atomic_dec(&priv->tx_inflight);
> +}
> +
> +/* Create a scatter-gather list representing our input buffer and put
> + * it in the queue.
> + *
> + * Callers should take appropriate locks.
> + */
> +static int virtio_can_add_inbuf(struct virtqueue *vq, void *buf,
> + unsigned int size)
> +{
> + struct scatterlist sg[1];
> + int ret;
> +
> + sg_init_one(sg, buf, size);
> +
> + ret = virtqueue_add_inbuf(vq, sg, 1, buf, GFP_ATOMIC);
> +
> + return ret;
> +}
> +
> +/* Send a control message with message type either
> + *
> + * - VIRTIO_CAN_SET_CTRL_MODE_START or
> + * - VIRTIO_CAN_SET_CTRL_MODE_STOP.
> + *
> + */
> +static u8 virtio_can_send_ctrl_msg(struct net_device *ndev, u16 msg_type)
> +{
> + struct scatterlist sg_out, sg_in, *sgs[2] = { &sg_out, &sg_in };
> + struct virtio_can_priv *priv = netdev_priv(ndev);
> + struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_CONTROL];
> + struct device *dev = &priv->vdev->dev;
> + unsigned int len;
> + int err;
> +
> + if (!vq)
> + return VIRTIO_CAN_RESULT_NOT_OK;
> +
> + guard(mutex)(&priv->ctrl_lock);
> +
> + priv->can_ctr_msg.cpkt_out.msg_type = cpu_to_le16(msg_type);
> + sg_init_one(&sg_out, &priv->can_ctr_msg.cpkt_out,
> + sizeof(priv->can_ctr_msg.cpkt_out));
> + sg_init_one(&sg_in, &priv->can_ctr_msg.cpkt_in, sizeof(priv->can_ctr_msg.cpkt_in));
> +
> + reinit_completion(&priv->ctrl_done);
> +
> + err = virtqueue_add_sgs(vq, sgs, 1u, 1u, priv, GFP_ATOMIC);
> + if (err != 0) {
> + dev_err(dev, "%s(): virtqueue_add_sgs() failed\n", __func__);
> + return VIRTIO_CAN_RESULT_NOT_OK;
> + }
> +
> + if (!virtqueue_kick(vq)) {
> + dev_err(dev, "%s(): Kick failed\n", __func__);
> + return VIRTIO_CAN_RESULT_NOT_OK;
> + }
> +
> + while (!virtqueue_get_buf(vq, &len) && !virtqueue_is_broken(vq))
> + wait_for_completion(&priv->ctrl_done);
> +
> + return priv->can_ctr_msg.cpkt_in.result;
> +}
> +
> +static int virtio_can_start(struct net_device *ndev)
> +{
> + struct virtio_can_priv *priv = netdev_priv(ndev);
> + u8 result;
> +
> + result = virtio_can_send_ctrl_msg(ndev, VIRTIO_CAN_SET_CTRL_MODE_START);
> + if (result != VIRTIO_CAN_RESULT_OK) {
> + netdev_err(ndev, "CAN controller start failed\n");
> + return -EIO;
> + }
> +
> + priv->busoff_pending = false;
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + return 0;
> +}
> +
> +static int virtio_can_set_mode(struct net_device *dev, enum can_mode mode)
> +{
> + int err;
> +
> + switch (mode) {
> + case CAN_MODE_START:
> + err = virtio_can_start(dev);
> + if (err)
> + return err;
> + netif_wake_queue(dev);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int virtio_can_open(struct net_device *ndev)
> +{
> + struct virtio_can_priv *priv = netdev_priv(ndev);
> + int err;
> +
> + err = open_candev(ndev);
> + if (err)
> + return err;
> +
> + err = virtio_can_start(ndev);
> + if (err) {
> + close_candev(ndev);
> + return err;
> + }
> +
> + virtio_can_napi_enable(priv);
> + netif_start_queue(ndev);
> +
> + return 0;
> +}
> +
> +static int virtio_can_stop(struct net_device *ndev)
> +{
> + struct virtio_can_priv *priv = netdev_priv(ndev);
> + struct device *dev = &priv->vdev->dev;
> + u8 result;
> +
> + result = virtio_can_send_ctrl_msg(ndev, VIRTIO_CAN_SET_CTRL_MODE_STOP);
> + if (result != VIRTIO_CAN_RESULT_OK) {
> + dev_err(dev, "CAN controller stop failed\n");
> + return -EIO;
> + }
> +
> + priv->busoff_pending = false;
> + priv->can.state = CAN_STATE_STOPPED;
> +
> + /* Switch carrier off if device was connected to the bus */
> + if (netif_carrier_ok(ndev))
> + netif_carrier_off(ndev);
> +
> + return 0;
> +}
> +
> +static int virtio_can_close(struct net_device *dev)
> +{
> + struct virtio_can_priv *priv = netdev_priv(dev);
> +
> + netif_stop_queue(dev);
> + /* Ignore stop error: ndo_stop must always complete cleanup regardless.
> + * virtio_can_stop() already logs the error if it fails.
> + */
> + virtio_can_stop(dev);
> + virtio_can_napi_disable(priv);
> + close_candev(dev);
> +
> + return 0;
> +}
> +
> +static netdev_tx_t virtio_can_start_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct scatterlist sg_out, sg_in, *sgs[2] = { &sg_out, &sg_in };
> + const unsigned int hdr_size = sizeof(struct virtio_can_tx_out);
> + struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> + struct virtio_can_priv *priv = netdev_priv(dev);
> + struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
> + netdev_tx_t xmit_ret = NETDEV_TX_OK;
> + struct virtio_can_tx *can_tx_msg;
> + u32 can_flags;
> + int putidx;
> + int err;
> +
> + if (can_dev_dropped_skb(dev, skb))
> + goto kick; /* No way to return NET_XMIT_DROP here */
> +
> + /* No local check for CAN_RTR_FLAG or FD frame against negotiated
> + * features. The device will reject those anyway if not supported.
> + */
> +
> + can_tx_msg = kzalloc(sizeof(*can_tx_msg) + cf->len, GFP_ATOMIC);
> + if (!can_tx_msg) {
> + kfree_skb(skb);
> + dev->stats.tx_dropped++;
> + goto kick; /* No way to return NET_XMIT_DROP here */
> + }
> +
> + can_tx_msg->tx_out.msg_type = cpu_to_le16(VIRTIO_CAN_TX);
> + can_tx_msg->tx_out.length = cpu_to_le16(cf->len);
> + can_flags = 0;
> +
> + if (cf->can_id & CAN_EFF_FLAG) {
> + can_flags |= VIRTIO_CAN_FLAGS_EXTENDED;
> + can_tx_msg->tx_out.can_id = cpu_to_le32(cf->can_id & CAN_EFF_MASK);
> + } else {
> + can_tx_msg->tx_out.can_id = cpu_to_le32(cf->can_id & CAN_SFF_MASK);
> + }
> + if (cf->can_id & CAN_RTR_FLAG)
> + can_flags |= VIRTIO_CAN_FLAGS_RTR;
> + else
> + memcpy(can_tx_msg->tx_out.sdu, cf->data, cf->len);
> + if (can_is_canfd_skb(skb))
> + can_flags |= VIRTIO_CAN_FLAGS_FD;
> +
> + can_tx_msg->tx_out.flags = cpu_to_le32(can_flags);
> +
> + sg_init_one(&sg_out, &can_tx_msg->tx_out, hdr_size + cf->len);
> + sg_init_one(&sg_in, &can_tx_msg->tx_in, sizeof(can_tx_msg->tx_in));
> +
> + putidx = virtio_can_alloc_tx_idx(priv);
> +
> + if (unlikely(putidx < 0)) {
> + /* -ENOMEM or -ENOSPC here. -ENOSPC should not be possible as
> + * tx_inflight >= can.echo_skb_max is checked in flow control
> + */
> + WARN_ON_ONCE(putidx == -ENOSPC);
> + kfree(can_tx_msg);
> + kfree_skb(skb);
> + dev->stats.tx_dropped++;
> + goto kick; /* No way to return NET_XMIT_DROP here */
> + }
> +
> + can_tx_msg->putidx = (unsigned int)putidx;
> +
> + /* Push loopback echo. Will be looped back on TX interrupt/TX NAPI */
> + err = can_put_echo_skb(skb, dev, can_tx_msg->putidx, 0);
> + if (unlikely(err)) {
> + /* skb was already freed by can_put_echo_skb() on error */
> + virtio_can_free_tx_idx(priv, can_tx_msg->putidx);
> + kfree(can_tx_msg);
> + dev->stats.tx_dropped++;
> + goto kick;
> + }
> +
> + /* Protect queue and list operations */
> + scoped_guard(spinlock_irqsave, &priv->tx_lock)
> + err = virtqueue_add_sgs(vq, sgs, 1u, 1u, can_tx_msg, GFP_ATOMIC);
> +
> + if (unlikely(err)) {
> + /*
> + * can_put_echo_skb() already consumed skb via consume_skb(),
> + * so returning NETDEV_TX_BUSY would cause the stack to requeue
> + * a freed pointer. Drop the frame and return OK instead.
> + */
> + can_free_echo_skb(dev, can_tx_msg->putidx, NULL);
> + virtio_can_free_tx_idx(priv, can_tx_msg->putidx);
> + netif_stop_queue(dev);
> + kfree(can_tx_msg);
> + dev->stats.tx_dropped++;
> + /* Expected never to be seen */
> + netdev_warn(dev, "TX: Stop queue, err = %d\n", err);
> + goto kick;
> + }
> +
> + /* Normal flow control: stop queue when no transmission slots left */
> + if (atomic_read(&priv->tx_inflight) >= priv->can.echo_skb_max ||
> + vq->num_free == 0 || (vq->num_free < ARRAY_SIZE(sgs) &&
> + !virtio_has_feature(vq->vdev, VIRTIO_RING_F_INDIRECT_DESC))) {
> + netif_stop_queue(dev);
> + netdev_dbg(dev, "TX: Normal stop queue\n");
> + }
> +
> +kick:
> + if (netif_queue_stopped(dev) || !netdev_xmit_more()) {
> + scoped_guard(spinlock_irqsave, &priv->tx_lock) {
> + if (!virtqueue_kick(vq))
> + netdev_err(dev, "%s(): Kick failed\n", __func__);
> + }
> + }
> +
> + return xmit_ret;
> +}
> +
> +static const struct net_device_ops virtio_can_netdev_ops = {
> + .ndo_open = virtio_can_open,
> + .ndo_stop = virtio_can_close,
> + .ndo_start_xmit = virtio_can_start_xmit,
> +};
> +
> +static int register_virtio_can_dev(struct net_device *dev)
> +{
> + dev->flags |= IFF_ECHO; /* we support local echo */
> + dev->netdev_ops = &virtio_can_netdev_ops;
> +
> + return register_candev(dev);
> +}
> +
> +static int virtio_can_read_tx_queue(struct virtqueue *vq)
> +{
> + struct virtio_can_priv *can_priv = vq->vdev->priv;
> + struct net_device *dev = can_priv->dev;
> + struct virtio_can_tx *can_tx_msg;
> + struct net_device_stats *stats;
> + unsigned int len;
> + u8 result;
> +
> + stats = &dev->stats;
> +
> + scoped_guard(spinlock_irqsave, &can_priv->tx_lock)
> + can_tx_msg = virtqueue_get_buf(vq, &len);
> +
> + if (!can_tx_msg)
> + return 0;
> +
> + if (unlikely(len < sizeof(struct virtio_can_tx_in))) {
> + netdev_err(dev, "TX ACK: Device sent no result code\n");
> + result = VIRTIO_CAN_RESULT_NOT_OK; /* Keep things going */
> + } else {
> + result = can_tx_msg->tx_in.result;
> + }
> +
> + if (can_priv->can.state < CAN_STATE_BUS_OFF) {
> + if (result != VIRTIO_CAN_RESULT_OK) {
> + struct can_frame *skb_cf;
> + struct sk_buff *skb = alloc_can_err_skb(dev, &skb_cf);
> +
> + if (skb) {
> + skb_cf->can_id |= CAN_ERR_CRTL;
> + skb_cf->data[1] |= CAN_ERR_CRTL_UNSPEC;
> + netif_rx(skb);
> + }
> + netdev_warn(dev, "TX ACK: Result = %u\n", result);
> + can_free_echo_skb(dev, can_tx_msg->putidx, NULL);
> + stats->tx_dropped++;
> + } else {
> + stats->tx_bytes += can_get_echo_skb(dev, can_tx_msg->putidx,
> + NULL);
> + stats->tx_packets++;
> + }
> + } else {
> + netdev_dbg(dev, "TX ACK: Controller inactive, drop echo\n");
> + can_free_echo_skb(dev, can_tx_msg->putidx, NULL);
> + stats->tx_dropped++;
> + }
> +
> + virtio_can_free_tx_idx(can_priv, can_tx_msg->putidx);
> +
> + /* Flow control */
> + if (netif_queue_stopped(dev)) {
> + netdev_dbg(dev, "TX ACK: Wake up stopped queue\n");
> + netif_wake_queue(dev);
> + }
> +
> + kfree(can_tx_msg);
> +
> + return 1; /* Queue was not empty so there may be more data */
> +}
> +
> +static int virtio_can_tx_poll(struct napi_struct *napi, int quota)
> +{
> + struct net_device *dev = napi->dev;
> + struct virtio_can_priv *priv = netdev_priv(dev);
> + struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
> + int work_done = 0;
> +
> + while (work_done < quota && virtio_can_read_tx_queue(vq) != 0)
> + work_done++;
> +
> + if (work_done < quota)
> + virtqueue_napi_complete(napi, vq, work_done);
> +
> + return work_done;
> +}
> +
> +static void virtio_can_tx_intr(struct virtqueue *vq)
> +{
> + struct virtio_can_priv *can_priv = vq->vdev->priv;
> +
> + virtqueue_disable_cb(vq);
> + napi_schedule(&can_priv->napi_tx);
> +}
> +
> +/* This function is the NAPI RX poll function and NAPI guarantees that this
> + * function is not invoked simultaneously on multiple processors.
> + * Read a RX message from the used queue and sends it to the upper layer.
> + */
> +static int virtio_can_read_rx_queue(struct virtqueue *vq)
> +{
> + const unsigned int header_size = sizeof(struct virtio_can_rx);
> + struct virtio_can_priv *priv = vq->vdev->priv;
> + struct net_device *dev = priv->dev;
> + struct net_device_stats *stats;
> + struct virtio_can_rx *can_rx;
> + unsigned int transport_len;
> + struct canfd_frame *cf;
> + struct sk_buff *skb;
> + unsigned int len;
> + u32 can_flags;
> + u16 msg_type;
> + u32 can_id;
> + int ret;
> +
> + stats = &dev->stats;
> +
> + can_rx = virtqueue_get_buf(vq, &transport_len);
> + if (!can_rx)
> + return 0; /* No more data */
> +
> + if (transport_len < header_size) {
> + netdev_warn(dev, "RX: Message too small\n");
> + goto putback;
> + }
> +
> + if (priv->can.state >= CAN_STATE_ERROR_PASSIVE) {
> + netdev_dbg(dev, "%s(): Controller not active\n", __func__);
> + goto putback;
> + }
> +
> + msg_type = le16_to_cpu(can_rx->msg_type);
> + if (msg_type != VIRTIO_CAN_RX) {
> + netdev_warn(dev, "RX: Got unknown msg_type %04x\n", msg_type);
> + goto putback;
> + }
> +
> + len = le16_to_cpu(can_rx->length);
> + can_flags = le32_to_cpu(can_rx->flags);
> + can_id = le32_to_cpu(can_rx->can_id);
> +
> + if (can_flags & ~CAN_KNOWN_FLAGS) {
> + stats->rx_dropped++;
> + netdev_warn(dev, "RX: CAN Id 0x%08x: Invalid flags 0x%x\n",
> + can_id, can_flags);
> + goto putback;
> + }
> +
> + if (can_flags & VIRTIO_CAN_FLAGS_EXTENDED) {
> + can_id &= CAN_EFF_MASK;
> + can_id |= CAN_EFF_FLAG;
> + } else {
> + can_id &= CAN_SFF_MASK;
> + }
> +
> + if (can_flags & VIRTIO_CAN_FLAGS_RTR) {
> + if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_RTR_FRAMES)) {
> + stats->rx_dropped++;
> + netdev_warn(dev, "RX: CAN Id 0x%08x: RTR not negotiated\n",
> + can_id);
> + goto putback;
> + }
> + if (can_flags & VIRTIO_CAN_FLAGS_FD) {
> + stats->rx_dropped++;
> + netdev_warn(dev, "RX: CAN Id 0x%08x: RTR with FD not possible\n",
> + can_id);
> + goto putback;
> + }
> +
> + if (len > 0xF) {
> + stats->rx_dropped++;
> + netdev_warn(dev, "RX: CAN Id 0x%08x: RTR with DLC > 0xF\n",
> + can_id);
> + goto putback;
> + }
> +
> + if (len > 0x8)
> + len = 0x8;
> +
> + can_id |= CAN_RTR_FLAG;
> + }
> +
> + if (transport_len < header_size + len) {
> + netdev_warn(dev, "RX: Message too small for payload\n");
> + goto putback;
> + }
> +
> + if (can_flags & VIRTIO_CAN_FLAGS_FD) {
> + if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_CAN_FD)) {
> + stats->rx_dropped++;
> + netdev_warn(dev, "RX: CAN Id 0x%08x: FD not negotiated\n",
> + can_id);
> + goto putback;
> + }
> +
> + if (len > CANFD_MAX_DLEN)
> + len = CANFD_MAX_DLEN;
> +
> + skb = alloc_canfd_skb(priv->dev, &cf);
> + } else {
> + if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_CAN_CLASSIC)) {
> + stats->rx_dropped++;
> + netdev_warn(dev, "RX: CAN Id 0x%08x: classic not negotiated\n",
> + can_id);
> + goto putback;
> + }
> +
> + if (len > CAN_MAX_DLEN)
> + len = CAN_MAX_DLEN;
> +
> + skb = alloc_can_skb(priv->dev, (struct can_frame **)&cf);
> + }
> + if (!skb) {
> + stats->rx_dropped++;
> + netdev_warn(dev, "RX: No skb available\n");
> + goto putback;
> + }
> +
> + cf->can_id = can_id;
> + cf->len = len;
> + if (!(can_flags & VIRTIO_CAN_FLAGS_RTR)) {
> + /* RTR frames have a DLC but no payload */
> + memcpy(cf->data, can_rx->sdu, len);
> + }
> +
> + if (netif_receive_skb(skb) == NET_RX_SUCCESS) {
> + stats->rx_packets++;
> + if (!(can_flags & VIRTIO_CAN_FLAGS_RTR))
> + stats->rx_bytes += len;
> + }
> +
> +putback:
> + /* Put processed RX buffer back into avail queue */
> + ret = virtio_can_add_inbuf(vq, can_rx,
> + priv->rpkt_len);
> + if (!ret)
> + virtqueue_kick(vq);
> + return 1; /* Queue was not empty so there may be more data */
> +}
> +
> +static int virtio_can_handle_busoff(struct net_device *dev)
> +{
> + struct virtio_can_priv *priv = netdev_priv(dev);
> + struct can_frame *cf;
> + struct sk_buff *skb;
> +
> + if (!priv->busoff_pending)
> + return 0;
> +
> + if (priv->can.state < CAN_STATE_BUS_OFF) {
> + netdev_dbg(dev, "entered error bus off state\n");
> +
> + /* bus-off state */
> + priv->can.state = CAN_STATE_BUS_OFF;
> + priv->can.can_stats.bus_off++;
> + can_bus_off(dev);
> + }
> +
> + /* propagate the error condition to the CAN stack */
> + skb = alloc_can_err_skb(dev, &cf);
> + if (unlikely(!skb))
> + return 0;
> +
> + /* bus-off state */
> + cf->can_id |= CAN_ERR_BUSOFF;
> +
> + /* Ensure that the BusOff indication does not get lost */
> + if (netif_receive_skb(skb) == NET_RX_SUCCESS)
> + priv->busoff_pending = false;
> +
> + return 1;
> +}
> +
> +static int virtio_can_rx_poll(struct napi_struct *napi, int quota)
> +{
> + struct net_device *dev = napi->dev;
> + struct virtio_can_priv *priv = netdev_priv(dev);
> + struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_RX];
> + int work_done = 0;
> +
> + work_done += virtio_can_handle_busoff(dev);
> +
> + while (work_done < quota && virtio_can_read_rx_queue(vq) != 0)
> + work_done++;
> +
> + if (work_done < quota)
> + virtqueue_napi_complete(napi, vq, work_done);
> +
> + return work_done;
> +}
> +
> +static void virtio_can_rx_intr(struct virtqueue *vq)
> +{
> + struct virtio_can_priv *can_priv = vq->vdev->priv;
> +
> + virtqueue_disable_cb(vq);
> + napi_schedule(&can_priv->napi);
> +}
> +
> +static void virtio_can_control_intr(struct virtqueue *vq)
> +{
> + struct virtio_can_priv *can_priv = vq->vdev->priv;
> +
> + complete(&can_priv->ctrl_done);
> +}
> +
> +static void virtio_can_config_changed(struct virtio_device *vdev)
> +{
> + struct virtio_can_priv *can_priv = vdev->priv;
> + u16 status;
> +
> + status = virtio_cread16(vdev, offsetof(struct virtio_can_config,
> + status));
> +
> + if (!(status & VIRTIO_CAN_S_CTRL_BUSOFF))
> + return;
> +
> + if (!can_priv->busoff_pending &&
> + can_priv->can.state < CAN_STATE_BUS_OFF) {
> + can_priv->busoff_pending = true;
> + napi_schedule(&can_priv->napi);
> + }
> +}
> +
> +static void virtio_can_populate_rx_vq(struct virtio_device *vdev)
> +{
> + struct virtio_can_priv *priv = vdev->priv;
> + struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_RX];
> + unsigned int buf_size = priv->rpkt_len;
> + int num_elements = vq->num_free;
> + u8 *buf = (u8 *)priv->rpkt;
> + unsigned int idx;
> + int ret = 0;
> +
> + for (idx = 0; idx < num_elements; idx++) {
> + ret = virtio_can_add_inbuf(vq, buf, buf_size);
> + if (ret < 0) {
> + dev_dbg(&vdev->dev, "rpkt fill: ret=%d, idx=%u, size=%u\n",
> + ret, idx, buf_size);
> + break;
> + }
> + buf += buf_size;
> + }
> +
> + if (idx > 0)
> + virtqueue_kick(vq);
> +
> + dev_dbg(&vdev->dev, "%u rpkt added\n", idx);
> +}
> +
> +static int virtio_can_find_vqs(struct virtio_can_priv *priv)
> +{
> + struct virtqueue_info vqs_info[] = {
> + { "can-tx", virtio_can_tx_intr },
> + { "can-rx", virtio_can_rx_intr },
> + { "can-state-ctrl", virtio_can_control_intr },
> + };
> +
> + /* Find the queues. */
> + return virtio_find_vqs(priv->vdev, VIRTIO_CAN_QUEUE_COUNT, priv->vqs,
> + vqs_info, NULL);
> +}
> +
> +/* Function must not be called before virtio_can_find_vqs() has been run */
> +static void virtio_can_del_vq(struct virtio_device *vdev)
> +{
> + struct virtio_can_priv *priv = vdev->priv;
> + struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
> + struct virtio_can_tx *can_tx_msg;
> + int q;
> +
> + if (!vq)
> + return;
> +
> + /* Reset the device */
> + virtio_reset_device(vdev);
> +
> + /* From here we have dead silence from the device side so no locks
> + * are needed to protect against device side events.
> + */
> +
> + /* Free pending TX buffers which were allocated in virtio_can_start_xmit() */
> + while ((can_tx_msg = virtqueue_detach_unused_buf(vq))) {
> + can_free_echo_skb(priv->dev, can_tx_msg->putidx, NULL);
> + virtio_can_free_tx_idx(priv, can_tx_msg->putidx);
> + kfree(can_tx_msg);
> + }
> +
> + /* RX and control queue buffers are managed elsewhere, just detach */
> + for (q = VIRTIO_CAN_QUEUE_RX; q < VIRTIO_CAN_QUEUE_COUNT; q++)
> + while (virtqueue_detach_unused_buf(priv->vqs[q]))
> + ;
> +
> + if (vdev->config->del_vqs)
> + vdev->config->del_vqs(vdev);
> +
> + memset(priv->vqs, 0, sizeof(priv->vqs));
> +}
> +
> +static void virtio_can_remove(struct virtio_device *vdev)
> +{
> + struct virtio_can_priv *priv = vdev->priv;
> + struct net_device *dev = priv->dev;
> +
> + unregister_candev(dev);
> +
> + virtio_can_del_vq(vdev);
> +
> + virtio_can_free_candev(dev);
> +}
> +
> +static int virtio_can_validate(struct virtio_device *vdev)
> +{
> + /* CAN needs always access to the config space.
> + * Check that the driver can access the config space
> + */
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config access disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> + dev_err(&vdev->dev,
> + "device does not comply with spec version 1.x\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int virtio_can_probe(struct virtio_device *vdev)
> +{
> + struct virtio_can_priv *priv;
> + struct net_device *dev;
> + size_t size;
> + int err;
> +
> + dev = alloc_candev(sizeof(struct virtio_can_priv),
> + VIRTIO_CAN_ECHO_SKB_MAX);
> + if (!dev)
> + return -ENOMEM;
> +
> + priv = netdev_priv(dev);
> +
> + ida_init(&priv->tx_putidx_ida);
> +
> + netif_napi_add(dev, &priv->napi, virtio_can_rx_poll);
> + netif_napi_add(dev, &priv->napi_tx, virtio_can_tx_poll);
> +
> + SET_NETDEV_DEV(dev, &vdev->dev);
> +
> + priv->dev = dev;
> + priv->vdev = vdev;
> + vdev->priv = priv;
> +
> + priv->can.do_set_mode = virtio_can_set_mode;
> + priv->can.bittiming.bitrate = CAN_BITRATE_UNKNOWN;
> + /* Set Virtio CAN supported operations */
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
> + if (virtio_has_feature(vdev, VIRTIO_CAN_F_CAN_FD)) {
> + priv->can.fd.data_bittiming.bitrate = CAN_BITRATE_UNKNOWN;
> + err = can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD);
> + if (err != 0)
> + goto on_failure;
> + }
> +
> + /* Initialize virtqueues */
> + err = virtio_can_find_vqs(priv);
> + if (err != 0)
> + goto on_failure;
> +
> + spin_lock_init(&priv->tx_lock);
> + mutex_init(&priv->ctrl_lock);
> +
> + init_completion(&priv->ctrl_done);
> +
> + priv->rpkt_len = sizeof(struct virtio_can_rx);
> +
> + if (virtio_has_feature(vdev, VIRTIO_CAN_F_CAN_FD))
> + priv->rpkt_len += CANFD_MAX_DLEN;
> + else
> + priv->rpkt_len += CAN_MAX_DLEN;
> +
> + size = priv->rpkt_len * priv->vqs[VIRTIO_CAN_QUEUE_RX]->num_free;
> + priv->rpkt = devm_kzalloc(&vdev->dev, size, GFP_KERNEL);
> + if (!priv->rpkt) {
> + virtio_can_del_vq(vdev);
> + err = -ENOMEM;
> + goto on_failure;
> + }
> + virtio_can_populate_rx_vq(vdev);
> +
> + err = register_virtio_can_dev(dev);
> + if (err) {
> + virtio_can_del_vq(vdev);
> + goto on_failure;
> + }
> +
> + return 0;
> +
> +on_failure:
> + virtio_can_free_candev(dev);
> + return err;
> +}
> +
> +static int __maybe_unused virtio_can_freeze(struct virtio_device *vdev)
> +{
> + struct virtio_can_priv *priv = vdev->priv;
> + struct net_device *ndev = priv->dev;
> +
> + if (netif_running(ndev)) {
> + /* virtio_can_close() calls netif_stop_queue(), virtio_can_stop(),
> + * napi_disable() and close_candev(). Call it directly (not via
> + * dev_close()) to preserve IFF_UP so that netif_running() returns
> + * true in virtio_can_restore() and the device is brought back up.
> + */
> + virtio_can_close(ndev);
> + netif_device_detach(ndev);
> + }
> +
> + priv->can.state = CAN_STATE_SLEEPING;
> +
> + virtio_can_del_vq(vdev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused virtio_can_restore(struct virtio_device *vdev)
> +{
> + struct virtio_can_priv *priv = vdev->priv;
> + struct net_device *ndev = priv->dev;
> + size_t size;
> + int err;
> +
> + err = virtio_can_find_vqs(priv);
> + if (err != 0)
> + return err;
> +
> + size = priv->rpkt_len * priv->vqs[VIRTIO_CAN_QUEUE_RX]->num_free;
> + priv->rpkt = devm_krealloc(&vdev->dev, priv->rpkt, size, GFP_KERNEL | __GFP_ZERO);
> + if (!priv->rpkt) {
> + virtio_can_del_vq(vdev);
> + return -ENOMEM;
> + }
> + virtio_can_populate_rx_vq(vdev);
> +
> + if (netif_running(ndev)) {
> + /* virtio_can_open() calls open_candev(), virtio_can_start(),
> + * napi_enable() and netif_start_queue(). Call it directly (not
> + * via dev_open()) since IFF_UP is still set from before freeze.
> + */
> + err = virtio_can_open(ndev);
> + if (err) {
> + virtio_can_del_vq(vdev);
> + return err;
> + }
> + netif_device_attach(ndev);
> + } else {
> + priv->can.state = CAN_STATE_STOPPED;
> + }
> +
> + return 0;
> +}
> +
> +static struct virtio_device_id virtio_can_id_table[] = {
> + { VIRTIO_ID_CAN, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> +static unsigned int features[] = {
> + VIRTIO_CAN_F_CAN_CLASSIC,
> + VIRTIO_CAN_F_CAN_FD,
> + VIRTIO_CAN_F_LATE_TX_ACK,
> + VIRTIO_CAN_F_RTR_FRAMES,
> +};
> +
> +static struct virtio_driver virtio_can_driver = {
> + .feature_table = features,
> + .feature_table_size = ARRAY_SIZE(features),
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .id_table = virtio_can_id_table,
> + .validate = virtio_can_validate,
> + .probe = virtio_can_probe,
> + .remove = virtio_can_remove,
> + .config_changed = virtio_can_config_changed,
> +#ifdef CONFIG_PM_SLEEP
> + .freeze = virtio_can_freeze,
> + .restore = virtio_can_restore,
> +#endif
> +};
> +
> +module_virtio_driver(virtio_can_driver);
> +MODULE_DEVICE_TABLE(virtio, virtio_can_id_table);
> +
> +MODULE_AUTHOR("OpenSynergy GmbH");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("CAN bus driver for Virtio CAN controller");
> diff --git a/include/uapi/linux/virtio_can.h b/include/uapi/linux/virtio_can.h
> new file mode 100644
> index 000000000000..08d7e3e78776
> --- /dev/null
> +++ b/include/uapi/linux/virtio_can.h
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Copyright (C) 2021-2023 OpenSynergy GmbH
> + * Copyright Red Hat, Inc. 2025
> + */
> +#ifndef _LINUX_VIRTIO_VIRTIO_CAN_H
> +#define _LINUX_VIRTIO_VIRTIO_CAN_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_types.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +
> +/* Feature bit numbers */
> +#define VIRTIO_CAN_F_CAN_CLASSIC 0
> +#define VIRTIO_CAN_F_CAN_FD 1
> +#define VIRTIO_CAN_F_RTR_FRAMES 2
> +#define VIRTIO_CAN_F_LATE_TX_ACK 3
> +
> +/* CAN Result Types */
> +#define VIRTIO_CAN_RESULT_OK 0
> +#define VIRTIO_CAN_RESULT_NOT_OK 1
> +
> +/* CAN flags to determine type of CAN Id */
> +#define VIRTIO_CAN_FLAGS_EXTENDED 0x8000
> +#define VIRTIO_CAN_FLAGS_FD 0x4000
> +#define VIRTIO_CAN_FLAGS_RTR 0x2000
> +
> +#define VIRTIO_CAN_MAX_DLEN 64 // this is like CANFD_MAX_DLEN
> +
> +struct virtio_can_config {
> +#define VIRTIO_CAN_S_CTRL_BUSOFF (1u << 0) /* Controller BusOff */
> + /* CAN controller status */
> + __le16 status;
> +};
> +
> +/* TX queue message types */
> +struct virtio_can_tx_out {
> +#define VIRTIO_CAN_TX 0x0001
> + __le16 msg_type;
> + __le16 length; /* 0..8 CC, 0..64 CAN-FD, 0..2048 CAN-XL, 12 bits */
> + __u8 reserved_classic_dlc; /* If CAN classic length = 8 then DLC can be 8..15 */
> + __u8 padding;
> + __le16 reserved_xl_priority; /* May be needed for CAN XL priority */
> + __le32 flags;
> + __le32 can_id;
> + __u8 sdu[] __counted_by_le(length);
> +};
> +
> +struct virtio_can_tx_in {
> + __u8 result;
> +};
> +
> +/* RX queue message types */
> +struct virtio_can_rx {
> +#define VIRTIO_CAN_RX 0x0101
> + __le16 msg_type;
> + __le16 length; /* 0..8 CC, 0..64 CAN-FD, 0..2048 CAN-XL, 12 bits */
> + __u8 reserved_classic_dlc; /* If CAN classic length = 8 then DLC can be 8..15 */
> + __u8 padding;
> + __le16 reserved_xl_priority; /* May be needed for CAN XL priority */
> + __le32 flags;
> + __le32 can_id;
> + __u8 sdu[] __counted_by_le(length);
> +};
> +
> +/* Control queue message types */
> +struct virtio_can_control_out {
> +#define VIRTIO_CAN_SET_CTRL_MODE_START 0x0201
> +#define VIRTIO_CAN_SET_CTRL_MODE_STOP 0x0202
> + __le16 msg_type;
> +};
> +
> +struct virtio_can_control_in {
> + __u8 result;
> +};
> +
> +#endif /* #ifndef _LINUX_VIRTIO_VIRTIO_CAN_H */
> --
> 2.42.0
^ permalink raw reply
* Re: [PATCH splitout] mm: memory-failure: serialize TestSetPageHWPoison with zone->lock
From: Miaohe Lin @ 2026-06-10 7:24 UTC (permalink / raw)
To: Michael S. Tsirkin, Zi Yan
Cc: David Hildenbrand (Arm), Andrew Morton, linux-kernel, Jason Wang,
Xuan Zhuo, Eugenio Pérez, Muchun Song, Oscar Salvador,
Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Baolin Wang, Nico Pache, Ryan Roberts, Dev Jain,
Barry Song, Lance Yang, Hugh Dickins, Matthew Brost, Joshua Hahn,
Rakie Kim, Byungchul Park, Gregory Price, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi
In-Reply-To: <20260609165829-mutt-send-email-mst@kernel.org>
On 2026/6/10 5:00, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2026 at 04:54:01PM -0400, Zi Yan wrote:
>> On 9 Jun 2026, at 16:34, Michael S. Tsirkin wrote:
>>
>>> On Tue, Jun 09, 2026 at 02:52:47PM -0400, Zi Yan wrote:
>>>> On 9 Jun 2026, at 14:39, Zi Yan wrote:
>>>>
>>>>> On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
>>>>>
>>>>>> On 6/9/26 20:10, Andrew Morton wrote:
>>>>>>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>>>>
>>>>>>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
>>>>>>>> update to page->flags can race with non-atomic flag operations
>>>>>>>> that run under zone->lock in the buddy allocator.
>>>>>>>>
>>>>>>>> In particular, __free_pages_prepare() does:
>>>>>>>>
>>>>>>>> page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>>>>>
>>>>>>>> This non-atomic read-modify-write, while correctly excluding
>>>>>>>> __PG_HWPOISON from the mask, can still lose a concurrent
>>>>>>>> TestSetPageHWPoison if the read happens before the poison bit
>>>>>>>> is set and the write happens after. Will only get worse if/when
>>>>>>>> we add more non-atomic flag operations.
>>>>>>>>
>>>>>>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
>>>>>>>> around ClearPageHWPoison in the retry path. This
>>>>>>>> serializes with all buddy flag manipulation. The cost is
>>>>>>>> negligible: one lock/unlock in an extremely rare path
>>>>>>>> (hardware memory errors).
>>>>>>>>
>>>>>>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
>>>>>>>> in this file operate on pages already removed from the buddy
>>>>>>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
>>>>>>>> need zone->lock protection.
>>>>>>>
>>>>>>> Sashiko is saying this doesn't do anything "Because
>>>>>>> __free_pages_prepare() executes entirely locklessly". Did it goof?
>>>>>>>
>>>>>>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
>>>>>>
>>>>>> Battle of the bots: it's right.
>>>>>
>>>>> Yep, __free_pages_prepare() changes the page flag without holding
>>>>> zone->lock.
>>>>
>>>> __free_pages_prepare() works on frozen pages and assumes no one else
>>>> touches the input page. To avoid this race, memory_failure() might
>>>> want to try_get_page() before TestClearPageHWPoison(), but I am not
>>>> sure if that works along with memory failure flow.
>>>>
>>>> Best Regards,
>>>> Yan, Zi
>>>
>>>
>>>
>>> Actually memory failure already plays with this down the road no?
>>>
>>> So maybe it's enough to just SetPageHWPoison afterwards again?
>>>
>>>
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index ee42d4361309..4758fea94a96 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -2415,6 +2415,7 @@ int memory_failure(unsigned long pfn, int flags)
>>> if (!res) {
>>> if (is_free_buddy_page(p)) {
>>> if (take_page_off_buddy(p)) {
>>> + SetPageHWPoison(p);
>>> page_ref_inc(p);
>>> res = MF_RECOVERED;
>>> } else {
>>>
>>>
>>> and maybe in a bunch of other places in there?
>>
>> You mean for fear of losing HWPoison flag in the earlier TestSetPageHWPoison(),
>> just set it again here?
>
> Yea.
>
>> Why not do it after get_hwpoison_page(), since that
>> is the expected page flag?
>
> It's still in the buddy at that point right? I'm worried buddy might
> poke at flags.
Since __free_pages_prepare() executes entirely locklessly, the only way to ensure
HWPoison flag won't be lost might be only set hwpoison flag iff we can make sure
pages are not on the way to buddy...
Thanks.
.
^ permalink raw reply
* Re: [PATCH splitout] mm: memory-failure: serialize TestSetPageHWPoison with zone->lock
From: Michael S. Tsirkin @ 2026-06-10 7:35 UTC (permalink / raw)
To: Miaohe Lin
Cc: Zi Yan, David Hildenbrand (Arm), Andrew Morton, linux-kernel,
Jason Wang, Xuan Zhuo, Eugenio Pérez, Muchun Song,
Oscar Salvador, Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka,
Mike Rapoport, Suren Baghdasaryan, Michal Hocko, Brendan Jackman,
Johannes Weiner, Baolin Wang, Nico Pache, Ryan Roberts, Dev Jain,
Barry Song, Lance Yang, Hugh Dickins, Matthew Brost, Joshua Hahn,
Rakie Kim, Byungchul Park, Gregory Price, Ying Huang,
Alistair Popple, Christoph Lameter, David Rientjes,
Roman Gushchin, Harry Yoo, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Chris Li, Kairui Song, Kemeng Shi, Nhat Pham, Baoquan He,
virtualization, linux-mm, Andrea Arcangeli, Naoya Horiguchi
In-Reply-To: <dd8d89be-9c73-8ff7-7d44-9705bddeda4d@huawei.com>
On Wed, Jun 10, 2026 at 03:24:30PM +0800, Miaohe Lin wrote:
> On 2026/6/10 5:00, Michael S. Tsirkin wrote:
> > On Tue, Jun 09, 2026 at 04:54:01PM -0400, Zi Yan wrote:
> >> On 9 Jun 2026, at 16:34, Michael S. Tsirkin wrote:
> >>
> >>> On Tue, Jun 09, 2026 at 02:52:47PM -0400, Zi Yan wrote:
> >>>> On 9 Jun 2026, at 14:39, Zi Yan wrote:
> >>>>
> >>>>> On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote:
> >>>>>
> >>>>>> On 6/9/26 20:10, Andrew Morton wrote:
> >>>>>>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>>>>>
> >>>>>>>> TestSetPageHWPoison() is called without zone->lock, so its atomic
> >>>>>>>> update to page->flags can race with non-atomic flag operations
> >>>>>>>> that run under zone->lock in the buddy allocator.
> >>>>>>>>
> >>>>>>>> In particular, __free_pages_prepare() does:
> >>>>>>>>
> >>>>>>>> page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> >>>>>>>>
> >>>>>>>> This non-atomic read-modify-write, while correctly excluding
> >>>>>>>> __PG_HWPOISON from the mask, can still lose a concurrent
> >>>>>>>> TestSetPageHWPoison if the read happens before the poison bit
> >>>>>>>> is set and the write happens after. Will only get worse if/when
> >>>>>>>> we add more non-atomic flag operations.
> >>>>>>>>
> >>>>>>>> Fix by acquiring zone->lock around TestSetPageHWPoison and
> >>>>>>>> around ClearPageHWPoison in the retry path. This
> >>>>>>>> serializes with all buddy flag manipulation. The cost is
> >>>>>>>> negligible: one lock/unlock in an extremely rare path
> >>>>>>>> (hardware memory errors).
> >>>>>>>>
> >>>>>>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere
> >>>>>>>> in this file operate on pages already removed from the buddy
> >>>>>>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not
> >>>>>>>> need zone->lock protection.
> >>>>>>>
> >>>>>>> Sashiko is saying this doesn't do anything "Because
> >>>>>>> __free_pages_prepare() executes entirely locklessly". Did it goof?
> >>>>>>>
> >>>>>>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git.mst@redhat.com
> >>>>>>
> >>>>>> Battle of the bots: it's right.
> >>>>>
> >>>>> Yep, __free_pages_prepare() changes the page flag without holding
> >>>>> zone->lock.
> >>>>
> >>>> __free_pages_prepare() works on frozen pages and assumes no one else
> >>>> touches the input page. To avoid this race, memory_failure() might
> >>>> want to try_get_page() before TestClearPageHWPoison(), but I am not
> >>>> sure if that works along with memory failure flow.
> >>>>
> >>>> Best Regards,
> >>>> Yan, Zi
> >>>
> >>>
> >>>
> >>> Actually memory failure already plays with this down the road no?
> >>>
> >>> So maybe it's enough to just SetPageHWPoison afterwards again?
> >>>
> >>>
> >>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> >>> index ee42d4361309..4758fea94a96 100644
> >>> --- a/mm/memory-failure.c
> >>> +++ b/mm/memory-failure.c
> >>> @@ -2415,6 +2415,7 @@ int memory_failure(unsigned long pfn, int flags)
> >>> if (!res) {
> >>> if (is_free_buddy_page(p)) {
> >>> if (take_page_off_buddy(p)) {
> >>> + SetPageHWPoison(p);
> >>> page_ref_inc(p);
> >>> res = MF_RECOVERED;
> >>> } else {
> >>>
> >>>
> >>> and maybe in a bunch of other places in there?
> >>
> >> You mean for fear of losing HWPoison flag in the earlier TestSetPageHWPoison(),
> >> just set it again here?
> >
> > Yea.
> >
> >> Why not do it after get_hwpoison_page(), since that
> >> is the expected page flag?
> >
> > It's still in the buddy at that point right? I'm worried buddy might
> > poke at flags.
>
> Since __free_pages_prepare() executes entirely locklessly, the only way to ensure
> HWPoison flag won't be lost might be only set hwpoison flag iff we can make sure
> pages are not on the way to buddy...
>
> Thanks.
> .
Right so here after take_page_off_buddy it's ok, for example.
--
MST
^ permalink raw reply
* [PATCH net-next] virtio-net: support xsk wake up
From: Menglong Dong @ 2026-06-10 8:16 UTC (permalink / raw)
To: xuanzhuo
Cc: mst, jasowang, eperezma, andrew+netdev, davem, edumazet, kuba,
pabeni, netdev, virtualization, linux-kernel
For now, XDP_RING_NEED_WAKEUP is not supported properly by the virtio-net.
Take the tx path for example, we set xsk_set_tx_need_wakeup() in
virtnet_xsk_xmit(), but we didn't call xsk_clear_tx_need_wakeup()
anywhere, which means the user will call send() for every packet.
For the tx path, we will call xsk_set_tx_need_wakeup() after
virtnet_xsk_xmit_batch() if sq->vq is empty, as we can't be wakeup by the
skb_xmit_done() in this case. Otherwise, we will clear the wakeup flag.
For the rx path, we will call xsk_set_rx_need_wakeup() if we have free
buffers in rq->vq.
Race condition is considered for both rx and tx path.
Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
drivers/net/virtio_net.c | 52 ++++++++++++++++++++++++++++++++++------
1 file changed, 45 insertions(+), 7 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9b3da9f9786c..25e895b849a6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1323,16 +1323,27 @@ static int virtnet_add_recvbuf_xsk(struct virtnet_info *vi, struct receive_queue
struct xsk_buff_pool *pool, gfp_t gfp)
{
struct xdp_buff **xsk_buffs;
+ bool need_wakeup;
dma_addr_t addr;
int err = 0;
u32 len, i;
int num;
+ need_wakeup = xsk_uses_need_wakeup(pool);
xsk_buffs = rq->xsk_buffs;
+ /* If both rq->vq and fill ring are empty, and then the user submit
+ * all the chunks to the fill ring and check the wake up flag
+ * after xsk_buff_alloc_batch() and before xsk_set_rx_need_wakeup(),
+ * we will lose the chance to wake up the rx napi, so we have to
+ * set the need_wakeup flag here.
+ */
+ if (need_wakeup && virtqueue_get_vring_size(rq->vq) == rq->vq->num_free)
+ xsk_set_rx_need_wakeup(pool);
+
num = xsk_buff_alloc_batch(pool, xsk_buffs, rq->vq->num_free);
if (!num) {
- if (xsk_uses_need_wakeup(pool)) {
+ if (need_wakeup) {
xsk_set_rx_need_wakeup(pool);
/* Return 0 instead of -ENOMEM so that NAPI is
* descheduled.
@@ -1341,8 +1352,6 @@ static int virtnet_add_recvbuf_xsk(struct virtnet_info *vi, struct receive_queue
}
return -ENOMEM;
- } else {
- xsk_clear_rx_need_wakeup(pool);
}
len = xsk_pool_get_rx_frame_size(pool) + vi->hdr_len;
@@ -1363,6 +1372,16 @@ static int virtnet_add_recvbuf_xsk(struct virtnet_info *vi, struct receive_queue
goto err;
}
+ if (need_wakeup) {
+ if (rq->vq->num_free)
+ /* We have free buffers, so we'd better wake up the
+ * rx napi as soon as possible.
+ */
+ xsk_set_rx_need_wakeup(pool);
+ else
+ xsk_clear_rx_need_wakeup(pool);
+ }
+
return num;
err:
@@ -1440,8 +1459,9 @@ static bool virtnet_xsk_xmit(struct send_queue *sq, struct xsk_buff_pool *pool,
struct virtnet_info *vi = sq->vq->vdev->priv;
struct virtnet_sq_free_stats stats = {};
struct net_device *dev = vi->dev;
+ int sent, vring_size;
+ bool need_wakeup;
u64 kicks = 0;
- int sent;
/* Avoid to wakeup napi meanless, so call __free_old_xmit instead of
* free_old_xmit().
@@ -1451,8 +1471,29 @@ static bool virtnet_xsk_xmit(struct send_queue *sq, struct xsk_buff_pool *pool,
if (stats.xsk)
xsk_tx_completed(sq->xsk_pool, stats.xsk);
+ vring_size = virtqueue_get_vring_size(sq->vq);
+ need_wakeup = xsk_uses_need_wakeup(pool);
+ /* If the sq->vq is empty, and the tx ring is empty, and the user
+ * submit an entry to the tx ring after virtnet_xsk_xmit_batch() and
+ * before xsk_set_tx_need_wakeup(), we will lose the chance to wake
+ * up the tx napi, so we have to set the need_wakeup flag here.
+ */
+ if (need_wakeup && vring_size == sq->vq->num_free)
+ xsk_set_tx_need_wakeup(pool);
+
sent = virtnet_xsk_xmit_batch(sq, pool, budget, &kicks);
+ if (need_wakeup) {
+ if (vring_size == sq->vq->num_free)
+ /* we can't wake up by ourself, and it should be done
+ * by the user.
+ */
+ xsk_set_tx_need_wakeup(pool);
+ else
+ /* we can wake up from skb_xmit_done() */
+ xsk_clear_tx_need_wakeup(pool);
+ }
+
if (!is_xdp_raw_buffer_queue(vi, sq - vi->sq))
check_sq_full_and_disable(vi, vi->dev, sq);
@@ -1470,9 +1511,6 @@ static bool virtnet_xsk_xmit(struct send_queue *sq, struct xsk_buff_pool *pool,
u64_stats_add(&sq->stats.xdp_tx, sent);
u64_stats_update_end(&sq->stats.syncp);
- if (xsk_uses_need_wakeup(pool))
- xsk_set_tx_need_wakeup(pool);
-
return sent;
}
--
2.54.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox