[PATCH] powerpc/sched: Cleanup vcpu_is_preempted()

[Aneesh Kumar K.V]

No functional change in this patch. A helper is added to find if
vcpu is dispatched by hypervisor. Use that instead of opencoding.
Also clarify some of the comments.

Signed-off-by: Aneesh Kumar K.V 
---
 arch/powerpc/include/asm/paravirt.h | 33 ++---
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/paravirt.h 
b/arch/powerpc/include/asm/paravirt.h
index ac4279208d63..b78b82d66057 100644
--- a/arch/powerpc/include/asm/paravirt.h
+++ b/arch/powerpc/include/asm/paravirt.h
@@ -76,6 +76,17 @@ static inline bool is_vcpu_idle(int vcpu)
 {
return lppaca_of(vcpu).idle;
 }
+
+static inline bool vcpu_is_dispatched(int vcpu)
+{
+   /*
+* This is the yield_count.  An "odd" value (low bit on) means that
+* the processor is yielded (either because of an OS yield or a
+* hypervisor preempt).  An even value implies that the processor is
+* currently executing.
+*/
+   return (!(yield_count_of(vcpu) & 1));
+}
 #else
 static inline bool is_shared_processor(void)
 {
@@ -109,6 +120,10 @@ static inline bool is_vcpu_idle(int vcpu)
 {
return false;
 }
+static inline bool vcpu_is_dispatched(int vcpu)
+{
+   return true;
+}
 #endif
 
 #define vcpu_is_preempted vcpu_is_preempted
@@ -134,12 +149,12 @@ static inline bool vcpu_is_preempted(int cpu)
 * If the hypervisor has dispatched the target CPU on a physical
 * processor, then the target CPU is definitely not preempted.
 */
-   if (!(yield_count_of(cpu) & 1))
+   if (vcpu_is_dispatched(cpu))
return false;
 
/*
-* If the target CPU has yielded to Hypervisor but OS has not
-* requested idle then the target CPU is definitely preempted.
+* if the target CPU is not dispatched and the guest OS
+* has not marked the CPU idle, then it is hypervisor preempted.
 */
if (!is_vcpu_idle(cpu))
return true;
@@ -166,7 +181,7 @@ static inline bool vcpu_is_preempted(int cpu)
 
/*
 * The PowerVM hypervisor dispatches VMs on a whole core
-* basis. So we know that a thread sibling of the local CPU
+* basis. So we know that a thread sibling of the executing CPU
 * cannot have been preempted by the hypervisor, even if it
 * has called H_CONFER, which will set the yield bit.
 */
@@ -174,15 +189,17 @@ static inline bool vcpu_is_preempted(int cpu)
return false;
 
/*
-* If any of the threads of the target CPU's core are not
-* preempted or ceded, then consider target CPU to be
-* non-preempted.
+* The specific target CPU was marked by guest OS as idle, but
+* then also check all other cpus in the core for PowerVM
+* because it does core scheduling and one of the vcpu
+* of the core getting preempted by hypervisor implies
+* other vcpus can also be considered preempted.
 */
first_cpu = cpu_first_thread_sibling(cpu);
for (i = first_cpu; i < first_cpu + threads_per_core; i++) {
if (i == cpu)
continue;
-   if (!(yield_count_of(i) & 1))
+   if (vcpu_is_dispatched(i))
return false;
if (!is_vcpu_idle(i))
return true;
-- 
2.41.0

[PATCH v2] powerpc/book3s/hash: Drop _PAGE_PRIVILEGED from PAGE_NONE

[Aneesh Kumar K.V]

There used to be a dependency on _PAGE_PRIVILEGED with pte_savedwrite.
But that got dropped by
commit 6a56ccbcf6c6 ("mm/autonuma: use can_change_(pte|pmd)_writable() to 
replace savedwrite")

With the change in this patch numa fault pte (pte_protnone()) gets mapped as 
regular user pte
with RWX cleared (no-access) whereas earlier it used to be mapped 
_PAGE_PRIVILEGED.

Hash fault handling code did get some WARN_ON added because those
functions are not expected to get called with _PAGE_READ cleared.
commit 18061c17c8ec ("powerpc/mm: Update PROTFAULT handling in the page fault 
path")
explains the details.

Also revert commit 1abce0580b89 ("powerpc/64s: Fix __pte_needs_flush() false 
positive warning")

Signed-off-by: Aneesh Kumar K.V 
---
 arch/powerpc/include/asm/book3s/64/pgtable.h  | 9 +++--
 arch/powerpc/include/asm/book3s/64/tlbflush.h | 9 ++---
 arch/powerpc/mm/book3s64/hash_utils.c | 7 +++
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h 
b/arch/powerpc/include/asm/book3s/64/pgtable.h
index cb77eddca54b..2cc58ac74080 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -17,12 +17,6 @@
 #define _PAGE_EXEC 0x1 /* execute permission */
 #define _PAGE_WRITE0x2 /* write access allowed */
 #define _PAGE_READ 0x4 /* read access allowed */
-#define _PAGE_NA   _PAGE_PRIVILEGED
-#define _PAGE_NAX  _PAGE_EXEC
-#define _PAGE_RO   _PAGE_READ
-#define _PAGE_ROX  (_PAGE_READ | _PAGE_EXEC)
-#define _PAGE_RW   (_PAGE_READ | _PAGE_WRITE)
-#define _PAGE_RWX  (_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC)
 #define _PAGE_PRIVILEGED   0x8 /* kernel access only */
 #define _PAGE_SAO  0x00010 /* Strong access order */
 #define _PAGE_NON_IDEMPOTENT   0x00020 /* non idempotent memory */
@@ -529,6 +523,9 @@ static inline bool pte_user(pte_t pte)
 }
 
 #define pte_access_permitted pte_access_permitted
+/*
+ * execute-only mappings return false
+ */
 static inline bool pte_access_permitted(pte_t pte, bool write)
 {
/*
diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h 
b/arch/powerpc/include/asm/book3s/64/tlbflush.h
index 1950c1b825b4..fd642b729775 100644
--- a/arch/powerpc/include/asm/book3s/64/tlbflush.h
+++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h
@@ -158,11 +158,6 @@ static inline void flush_tlb_fix_spurious_fault(struct 
vm_area_struct *vma,
 */
 }
 
-static inline bool __pte_protnone(unsigned long pte)
-{
-   return (pte & (pgprot_val(PAGE_NONE) | _PAGE_RWX)) == 
pgprot_val(PAGE_NONE);
-}
-
 static inline bool __pte_flags_need_flush(unsigned long oldval,
  unsigned long newval)
 {
@@ -179,8 +174,8 @@ static inline bool __pte_flags_need_flush(unsigned long 
oldval,
/*
 * We do not expect kernel mappings or non-PTEs or not-present PTEs.
 */
-   VM_WARN_ON_ONCE(!__pte_protnone(oldval) && oldval & _PAGE_PRIVILEGED);
-   VM_WARN_ON_ONCE(!__pte_protnone(newval) && newval & _PAGE_PRIVILEGED);
+   VM_WARN_ON_ONCE(oldval & _PAGE_PRIVILEGED);
+   VM_WARN_ON_ONCE(newval & _PAGE_PRIVILEGED);
VM_WARN_ON_ONCE(!(oldval & _PAGE_PTE));
VM_WARN_ON_ONCE(!(newval & _PAGE_PTE));
VM_WARN_ON_ONCE(!(oldval & _PAGE_PRESENT));
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c 
b/arch/powerpc/mm/book3s64/hash_utils.c
index ad2afa08e62e..0626a25b0d72 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -310,9 +310,16 @@ unsigned long htab_convert_pte_flags(unsigned long 
pteflags, unsigned long flags
else
rflags |= 0x3;
}
+   VM_WARN_ONCE(!(pteflags & _PAGE_RWX), "no-access mapping 
request");
} else {
if (pteflags & _PAGE_RWX)
rflags |= 0x2;
+   /*
+* We should never hit this in normal fault handling because
+* a permission check (check_pte_access()) will bubble this
+* to higher level linux handler even for PAGE_NONE.
+*/
+   VM_WARN_ONCE(!(pteflags & _PAGE_RWX), "no-access mapping 
request");
if (!((pteflags & _PAGE_WRITE) && (pteflags & _PAGE_DIRTY)))
rflags |= 0x1;
}
-- 
2.41.0

RE: [PATCH 3/3] PCI: dwc: Add dw_pcie_ep_{read,write}_dbi[2] helpers

[Yoshihiro Shimoda]

Hello Serge,

> From: Serge Semin, Sent: Monday, November 13, 2023 9:41 PM
> 
> On Mon, Nov 13, 2023 at 10:33:00AM +0900, Yoshihiro Shimoda wrote:
> > The current code calculated some dbi[2] registers' offset by calling
> > dw_pcie_ep_get_dbi[2]_offset() in each function. To improve code
> > readability, add dw_pcie_ep_{read,write}_dbi[2} and some data-width
> > related helpers.
> 
> Thanks for submitting this cleanup patch. That's exactly what I meant
> here

> and Mani later here

> 
> Please note a few nitpicks below.

Thank you for your review!

> >
> > Signed-off-by: Yoshihiro Shimoda 
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   | 230 ++
> >  1 file changed, 129 insertions(+), 101 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c 
> > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 1100671db887..dcbed49c9613 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -65,24 +65,89 @@ static unsigned int dw_pcie_ep_get_dbi2_offset(struct 
> > dw_pcie_ep *ep, u8 func_no
> > return dbi2_offset;
> >  }
> >
> > +static u32 dw_pcie_ep_read_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg,
> > +  size_t size)
> > +{
> > +   unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
> > +   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +   return dw_pcie_read_dbi(pci, offset + reg, size);
> > +}
> > +
> > +static void dw_pcie_ep_write_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 
> > reg,
> > +size_t size, u32 val)
> > +{
> > +   unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
> > +   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +   dw_pcie_write_dbi(pci, offset + reg, size, val);
> > +}
> > +
> > +static void dw_pcie_ep_write_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 
> > reg,
> > + size_t size, u32 val)
> > +{
> > +   unsigned int offset = dw_pcie_ep_get_dbi2_offset(ep, func_no);
> > +   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +   dw_pcie_write_dbi2(pci, offset + reg, size, val);
> > +}
> > +
> > +static inline void dw_pcie_ep_writel_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +u32 reg, u32 val)
> > +{
> > +   dw_pcie_ep_write_dbi(ep, func_no, reg, 0x4, val);
> > +}
> > +
> > +static inline u32 dw_pcie_ep_readl_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +  u32 reg)
> > +{
> > +   return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x4);
> > +}
> > +
> > +static inline void dw_pcie_ep_writew_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +u32 reg, u16 val)
> > +{
> > +   dw_pcie_ep_write_dbi(ep, func_no, reg, 0x2, val);
> > +}
> > +
> > +static inline u16 dw_pcie_ep_readw_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +  u32 reg)
> > +{
> > +   return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x2);
> > +}
> > +
> > +static inline void dw_pcie_ep_writeb_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +u32 reg, u8 val)
> > +{
> > +   dw_pcie_ep_write_dbi(ep, func_no, reg, 0x1, val);
> > +}
> > +
> > +static inline u8 dw_pcie_ep_readb_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > + u32 reg)
> > +{
> > +   return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x1);
> > +}
> > +
> > +static inline void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 
> > func_no,
> > + u32 reg, u32 val)
> > +{
> > +   dw_pcie_ep_write_dbi2(ep, func_no, reg, 0x4, val);
> > +}
> > +
> 
> I am not sure whether the methods above are supposed to be defined
> here instead of being moved to the "pcie-designware.h" header file
> together with dw_pcie_ep_get_dbi2_offset() and
> dw_pcie_ep_get_dbi_offset(). The later place seems more suitable
> seeing the accessors are generic, look similar to the
> dw_pcie_{write,read}_dbi{,2}() functions and might be useful in the
> platform drivers. On the other hand no LLDDs would have used it
> currently. So I'll leave this as a food for thoughts for the driver
> and subsystem maintainers.

Perhaps, when a device driver needs to use these functions actually,
we can move these functions to pcie-designware.h, I think.

> >  static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
> >enum pci_barno bar, int flags)
> >  {
> > -   unsigned int dbi_offset, dbi2_offset;
> > struct dw_pcie_ep *ep = &pci->ep;
> > u32 reg, reg_dbi2;
> >
> > -   dbi_offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
> > -   dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no);
> > -
> > -   reg = dbi_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
> > -   reg_dbi2 = dbi2_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
> 
> > +   reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> > +   reg_dbi2 = PCI_BASE_ADDRESS_0 + (4 * bar);
>

RE: [PATCH 0/3] PCI: dwc: Improve code readability

[Yoshihiro Shimoda]

Hello,

> From: Krzysztof Wilczyński, Sent: Monday, November 13, 2023 9:22 PM
> 
> [...]
> > > > Now, while you are looking at things, can you also take care about the 
> > > > following:
> > > >
> > > >   drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to 
> > > > smaller integer type 'enum
> dw_pcie_device_mode'
> > > from 'const void *' [-Wvoid-pointer-to-enum-cast]
> >
> > Thank you for the report!
> >
> > > > This requires adding structs for each data member of the of_device_id 
> > > > type.
> > >
> > > That sounds like overkill to me.
> > > An intermediate cast to uintptr_t should fix the issue as well.
> >
> > I confirmed that the uintptr_t fixed the issue.
> 
> We declined a similar fix in the past[1] ...
> 
> > I also think that adding a new struct with the mode is overkill.
> 
> ... with the hopes that a driver could drop the switch statements in place
> of using the other pattern.  Also, to be consistent with other drivers that
> do this already.
> 
> > So, I would like to fix the issue by using the cast of uintptr_t.
> 
> Sure.  I appreciate that this would be more work.  When you send your
> patch, can you include an update to the iproc driver (and credit the
> original author from [1])?  I would appreciate it.
> 
> 1. https://lore.kernel.org/linux-pci/20230814230008.GA196797@bhelgaas/

I got it. I'll include the following patch on v2.

https://lore.kernel.org/linux-pci/20230814-void-drivers-pci-controller-pcie-iproc-platform-v1-1-81a121607...@google.com/

Best regards,
Yoshihiro Shimoda

> 
>   Krzysztof

Fail to boot KCSAN-enabled kernel (Kernel panic - not syncing: Fatal exception, Unrecoverable FP Unavailable Exception 800 at c0000000022cafe0) on a PowerMac G5, kernel 6.6.1

[Erhard Furtner]

Greetings!

Both my PowerMac G5 and my Talos II (running a BE kernel+system) fail to boot a 
KCSAN-enabled kernel. Same kernel without KSCAN enabled boots just fine.

I tried to dig a little deeper with a stripped down .config on the G5 with 
CONFIG_KCSAN=y, CONFIG_KCSAN_STRICT=y and finally got some output via 
CONFIG_PPC_EARLY_DEBUG_G5=y. The machine freezes before output via serial 
console or netconsole is available so I had to take screen shots and 
transcribed them.

On EARLY_DEBUG_G5 the last output (there's some before but it gets overwritten) 
shown on the screen is:

[c22ebd90] [c00f95c4] __cpuhp_setup_state_cpuslocked+0x1b4/0x590
[c22ebd60] [c00f9ab8] __cpuhp_setup_state+0x118/0x2e8
[c22ebef0] [c2023dc8] poking_init+0x3c/0x90
[c22ebf10] [c20059a8] start_kernel+0x6a0/0x99c
[c22ebfe0] [c000cb48] start_here_common+0x1c/0x20
Code: 7fff9830 7ad6f082 3bff 7936f00e 7fffa838 7bff1f24 7e96fa15 41820218 
7e83a378 482eed09 6000 <7eb6f82a> 2c35 41820230 3921
---[ end trace  ]---

Kernel panic - not syncing: Fatal exception
Unrecoverable FP Unavailable Exception 800 at c22cafe0
Oops: Unrecoverable FP Unavailable Exception, sig: 6 [#2]
BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Tainted: G  D W
Hardware name: PowerMac11,2 PPC97OMP 0x440101 PowerMac
NIP:  c22cafe0 LR: c0046178 CTR: c22cafe0
REGS: c22eb290 TRAP: 0800   Tainted: G  D W   ()
MSR:  90001032   CR: 84048882  XER: 
IRQMASK: 3
GPR00:  c22eb530 c16cb100 fffe
GPR04:    
GPR08:    023404df
GPR12: c22cafe0 c2388000 0014aa88 
GPR16: ff9aac70 c03593e0 c03584a0 0009
GPR20: f886000f7ce74a00 0001  
GPR24: c22f8ab8 c23404d0 c22cbac0 fffe
GPR28: c23484d8 000f4240 c23404cc c23404c0
NIP [c22cafe0] init_task+0x9e0/0xf00
LR [00046170] __smp_send_nmi_ipi+0x4c0/0x610
Call Trace:
[c22eb530] [00046170] __smp_send_nmi_ipi+0x480/0x610 
(unreliable)
[c22eb5c0] [00046b60] smp_send_stop+0x30/0x60
[c22eb5e0] [000f5bb0] panic+0x274/0x554
[c22eb6a0] [0002313c] die+0x4bc/0x4c0
[c22eb760] [00062e98] bad_page_fault+0x200/0x2c0
[c22eb7f0] [00063148] do_bad_segment_interrupt+0x58/0xe0
[c22eb828] [7afc] data_access_slb_common_virt+0x19c/0x1a0
--- interrupt: 380 at hash__map_kernel_page+0x178/0x460
NIP:  c0069c48 LR: c0069c44 CTR: 
REGS: c22eb850 TRAP: 0380   Tainted: G  D W   ()
MSR:  90001032   CR: 44042882  XER: 
IRQMASK: 1
GPR00:  c22ebaf0 c16cb100 
GPR04:    
GPR08:    
GPR12:  c2388000 0014aa88 
GPR16: ff9aac70 c03593e0 c03584a0 0009
GPR20: f886000f7ce74a00 000c000cd000 f886000f7ce74a88 0007
GPR24: 0009 c23429e0 c23429e8 c22d8d80
GPR28: 818e 02322000 c23404cc 
NIP [c0069c48] hash__map_kernel_page+0x178/0x460
LR [c0069c44] hash__map_kernel_page+0x174/0x460
--- interrupt: 380
[c22ebaf0] [c22ebb80] init_stack+0x3b80/0x4000 (unreliable)
[c22ebbd0] [c0077d08] text_area_cpu_up+0x78/0x490
[c22ebc80] [c00f6608] cpuhp_invoke_callback+0x218/0x490
[c22ebcf0] [c00f8e28] cpuhp_issue_cal1+0x4c8/0x4f0
[c22ebd90] [c00f95c4] __cpuhp_setup_state_cpuslocked+0x1b4/0x590
[c22ebd60] [c00f9ab8] __cpuhp_setup_state+0x118/0x2e8
[c22ebef0] [c2023dc8] poking_init+0x3c/0x90
[c22ebf10] [c20059a8] start_kernel+0x6a0/0x99c
[c22ebfe0] [c000cb48] start_here_common+0x1c/0x20
Code: c000 0006f230    c004 7fe06608 c000 
023432a8  0001  022cb020  
---[ end trace  ]---

Kernel panic - not syncing: Fatal exception


When I additionally enable CONFIG_KCSAN_SELFTEST=y the machine freezes even 
earlier and I only get this:

ioremap() called early from iommu_init_early_dart+0x29c/0xb90. Use 
early_ioremap() instead
DART table allocated at: (ptrval)
DART IOMMU initialized for U4 type chipset
Hardware name: PowerMac11,2 PPC970MP 0x440101 PowerMac
CPU maps init

Re: [RFC PATCH] powerpc/book3s/hash: Drop _PAGE_PRIVILEGED from PAGE_NONE

[Aneesh Kumar K V]

On 11/13/23 5:17 PM, Nicholas Piggin wrote:
> On Mon Nov 13, 2023 at 8:45 PM AEST, Aneesh Kumar K V wrote:



 diff --git a/arch/powerpc/mm/book3s64/hash_utils.c 
 b/arch/powerpc/mm/book3s64/hash_utils.c
 index ad2afa08e62e..b2eda22195f0 100644
 --- a/arch/powerpc/mm/book3s64/hash_utils.c
 +++ b/arch/powerpc/mm/book3s64/hash_utils.c
 @@ -310,9 +310,26 @@ unsigned long htab_convert_pte_flags(unsigned long 
 pteflags, unsigned long flags
else
rflags |= 0x3;
}
 +  WARN_ON(!(pteflags & _PAGE_RWX));
} else {
if (pteflags & _PAGE_RWX)
rflags |= 0x2;
 +  else {
 +  /*
 +   * PAGE_NONE will get mapped to 0b110 (slb key 1 no 
 access)
 +   * We picked 0b110 instead of 0b000 so that slb key 0 
 will
 +   * get only read only access for the same rflags.
 +   */
 +  if (mmu_has_feature(MMU_FTR_KERNEL_RO))
 +  rflags |= (HPTE_R_PP0 | 0x2);
 +  /*
 +   * rflags = HPTE_R_N
 +   * Without KERNEL_RO feature this will result in slb
 +   * key 0 with read/write. But ISA only supports that.
 +   * There is no key 1 no-access and key 0 read-only
 +   * pp bit support.
 +   */
 +  }
if (!((pteflags & _PAGE_WRITE) && (pteflags & _PAGE_DIRTY)))
rflags |= 0x1;
}
>>>
>>
>> V2 is also dropping the above change, because we will never have hash table 
>> entries inserted. 
>>
>> This is added to commit message.
>>
>> Hash fault handling code did get some WARN_ON added because those
>> functions are not expected to get called with _PAGE_READ cleared.
>> commit 18061c17c8ec ("powerpc/mm: Update PROTFAULT handling in the page 
>> fault path")
>> explains the details.
> 
> Should it be a WARN_ON_ONCE? Any useful way to recover from it? Could
> the added WARN come with some comments from that commit that explain
> it?
> 

This should never happen unless someone messed up check_pte_access(). The 
WARN_ON() is a way
to remind me not to add no access ppp mapping in the htab_convert_pte_flags() 
as I did above.
Initially I was planning to add only a comment, but then in the rare case of we 
getting it wrong
it is nice to do a console log.

-aneesh

Re: [PATCH v3 00/10] powerpc/pseries: New character devices for system parameters and VPD

[Nathan Lynch]

Michal Suchánek  writes:
> What's the status here?
>
> Can this move on with the 4th patch skipped, or is new revision
> expected?

I would like to get some feedback on the idea for coarse per-function
locking in patch #2 "Facilitate high-level call sequences" since that is
a core change to the RTAS subsystem and new in this revision. But I
intend to send a new version (without a boot bug) this week regardless.

Re: [PATCH 0/3] PCI: dwc: Improve code readability

[Krzysztof Wilczyński]

Hello,

[...]
> > > I confirmed that the uintptr_t fixed the issue.
> >
> > We declined a similar fix in the past[1] ...
> >
> > > I also think that adding a new struct with the mode is overkill.
> >
> > ... with the hopes that a driver could drop the switch statements in place
> > of using the other pattern.  Also, to be consistent with other drivers that
> > do this already.
> 
> Note that the issue of casting is something we cannot fix easily:
> some *_device_id structs use "kernel_ulong_t" for the "data" member,
> others use "void *".
> 
> git grep -W "_device_id" -- include/linux/mod_devicetable.h | grep data
> 
> In addition, several drivers use multiple types of device IDs, so you
> cannot settle on one type to avoid casts.
> 
> Also, putting enum values in instances of that struct, as suggested,
> increases kernel size, for IMHO no additional gain.

All good points!  Thank you for taking the time to get back to me.  
Appreciated. :)

> If there is more data to put in the struct, I agree it makes sense to use
> a struct.

Yeah.  Perhaps if there is such a need in the future, indeed.

Krzysztof

Re: Fbdev issue after the drm updates 'drm-next-2023-10-31-1'

[Christian Zigotzky]

On 13 November 2023 at 01:48 pm, Geert Uytterhoeven wrote:

Hi Christian,

On Sun, Nov 12, 2023 at 3:23 PM Christian Zigotzky
 wrote:

On 07 November 2023 at 09:36 am, Christian Zigotzky wrote:

I have found out that fbdev no longer works with virtio-gpu-pci and
virtio-vga. It is not a problem with the penguin logos.

Could you please check fbdev in QEMU virtual machines with
virtio-gpu-pci and virtio-vga graphics?
On 02 November 2023 at 03:45 pm, Christian Zigotzky wrote:

There is a fbdev issue with the virtio-gpu-pci and virtio-vga. (The
penguins are not displayed at boot time)

Error message:  [0.889302] virtio-pci :00:02.0: [drm] *ERROR*
fbdev: Failed to setup generic emulation (ret=-2)

The kernel 6.6 final doesn't have this issue.

Please check the fbdev changes in the drm updates
'drm-next-2023-10-31-1'.

Thanks for your report!

I can confirm there is no graphics output with m68k/virt, and
bisected this to my own commit 6ae2ff23aa43a0c4 ("drm/client: Convert
drm_client_buffer_addfb() to drm_mode_addfb2()"), ouch...

It turns out the old call to drm_mode_addfb() caused a translation
from a fourcc to a bpp/depth pair to a _different_ fourcc, due to the
quirk processing in drm_driver_legacy_fb_format().
I.e. on m68k/virt, the original requested format was XR24, which was
translated to BX24. The former doesn't work, the latter works.

The following (gmail-whitespace-damaged) patch fixed the issue for me:

--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -400,6 +400,16 @@ static int drm_client_buffer_addfb(struct
drm_client_buffer *buffer,

 fb_req.width = width;
 fb_req.height = height;
+   if (client->dev->mode_config.quirk_addfb_prefer_host_byte_order) {
+   if (format == DRM_FORMAT_XRGB)
+   format = DRM_FORMAT_HOST_XRGB;
+   if (format == DRM_FORMAT_ARGB)
+   format = DRM_FORMAT_HOST_ARGB;
+   if (format == DRM_FORMAT_RGB565)
+   format = DRM_FORMAT_HOST_RGB565;
+   if (format == DRM_FORMAT_XRGB1555)
+   format = DRM_FORMAT_HOST_XRGB1555;
+   }
 fb_req.pixel_format = format;
 fb_req.handles[0] = handle;
 fb_req.pitches[0] = buffer->pitch;

However, I don't think we want to sprinkle more of these
translations around... So perhaps we should (re)add a call to
drm_driver_legacy_fb_format() to drm_client_buffer_addfb()?

Second, as I doubt you are using a big-endian system, you are probably
running into a slightly different issue.

Oh wait, you did CC linuxppc-dev, so perhaps you are running on a
big-endian machine?

If not, please add

 pr_info("%s: format = %p4cc\n", __func__, &format);

to drivers/gpu/drm/drm_client.c:drm_client_buffer_addfb(), and,
after reverting commit 6ae2ff23aa43a0c4, add

 pr_info("%s: bpp %u/depth %u => r.pixel_format = %p4cc\n",
__func__, or->bpp, or->depth, &r.pixel_format);

to drivers/gpu/drm/drm_framebuffer.c:drm_mode_addfb(), so we know the
translation in your case?

Thanks!

Gr{oetje,eeting}s,

 Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 -- Linus Torvalds

Hi Geert,

Thanks for your answer! I use a big-endian system.

Cheers,
Christian

Re: [PATCH 0/3] PCI: dwc: Improve code readability

[Geert Uytterhoeven]

Hi Krzysztof,

On Mon, Nov 13, 2023 at 1:22 PM Krzysztof Wilczyński  wrote:
> > > > Now, while you are looking at things, can you also take care about the 
> > > > following:
> > > >
> > > >   drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to 
> > > > smaller integer type 'enum dw_pcie_device_mode'
> > > from 'const void *' [-Wvoid-pointer-to-enum-cast]
> >
> > Thank you for the report!
> >
> > > > This requires adding structs for each data member of the of_device_id 
> > > > type.
> > >
> > > That sounds like overkill to me.
> > > An intermediate cast to uintptr_t should fix the issue as well.
> >
> > I confirmed that the uintptr_t fixed the issue.
>
> We declined a similar fix in the past[1] ...
>
> > I also think that adding a new struct with the mode is overkill.
>
> ... with the hopes that a driver could drop the switch statements in place
> of using the other pattern.  Also, to be consistent with other drivers that
> do this already.

Note that the issue of casting is something we cannot fix easily:
some *_device_id structs use "kernel_ulong_t" for the "data" member,
others use "void *".

git grep -W "_device_id" -- include/linux/mod_devicetable.h | grep data

In addition, several drivers use multiple types of device IDs, so you
cannot settle on one type to avoid casts.

Also, putting enum values in instances of that struct, as suggested,
increases kernel size, for IMHO no additional gain.  If there is more
data to put in the struct, I agree it makes sense to use a struct.

> > So, I would like to fix the issue by using the cast of uintptr_t.
>
> Sure.  I appreciate that this would be more work.  When you send your
> patch, can you include an update to the iproc driver (and credit the
> original author from [1])?  I would appreciate it.
>
> 1. https://lore.kernel.org/linux-pci/20230814230008.GA196797@bhelgaas/

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Laurent Pinchart]

Hi Hans,

On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
> On 13/11/2023 12:43, Laurent Pinchart wrote:
> > On Mon, Nov 13, 2023 at 11:28:51AM +, Sakari Ailus wrote:
> >> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> >>> On 13/11/2023 12:07, Laurent Pinchart wrote:
>  On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> > On 13/11/2023 11:42, Laurent Pinchart wrote:
> >> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>  Fixed point controls are used by the user to configure
>  a fixed point value in 64bits, which Q31.32 format.
> 
>  Signed-off-by: Shengjiu Wang 
> >>>
> >>> This patch adds a new control type. This is something that also needs 
> >>> to be
> >>> tested by v4l2-compliance, and for that we need to add support for 
> >>> this to
> >>> one of the media test-drivers. The best place for that is the vivid 
> >>> driver,
> >>> since that has already a bunch of test controls for other control 
> >>> types.
> >>>
> >>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> >>>
> >>> Can you add a patch adding a fixed point test control to vivid?
> >>
> >> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> >> relate more to units than control types. We have lots of fixed-point
> >> values in controls already, using the 32-bit and 64-bit integer control
> >> types. They use various locations for the decimal point, depending on
> >> the control. If we want to make this more explicit to users, we should
> >> work on adding unit support to the V4L2 controls.
> >
> > "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> 
>  It's not a unit, but I think it's related to units. My point is that,
>  without units support, I don't see why we need a formal definition of
>  fixed-point types, and why this series couldn't just use
>  VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
>  values as they see fit.
> >>>
> >>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> >>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> > 
> > Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> > 
> >>> is always interpreted as a 64 bit integer and nothing else. As it should.
> > 
> > The most common case for control handling in drivers is taking the
> > integer value and converting it to a register value, using
> > device-specific encoding of the register value. It can be a fixed-point
> > format or something else, depending on the device. My point is that
> > drivers routinely convert a "plain" integer to something else, and that
> > has never been considered as a cause of concern. I don't see why it
> > would be different in this series.
> > 
> >>> And while we do not have support for units (other than the documentation),
> >>> we do have type support in the form of V4L2_CTRL_TYPE_*.
> >>>
> > A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> > only shows a single driver specific control (dw100.rst).
> >
> > I'm not aware of other controls in mainline that use fixed point.
> 
>  The analog gain control for sensors for instance.
> >>>
> >>> Not really. The documentation is super vague:
> >>>
> >>> V4L2_CID_ANALOGUE_GAIN (integer)
> >>>
> >>>   Analogue gain is gain affecting all colour components in the pixel 
> >>> matrix. The
> >>>   gain operation is performed in the analogue domain before A/D 
> >>> conversion.
> >>>
> >>> And the integer is just a range. Internally it might map to some fixed
> >>> point value, but userspace won't see that, it's hidden in the driver 
> >>> AFAICT.
> > 
> > It's hidden so well that libcamera has a database of the sensor it
> > supports, with formulas to map a real gain value to the
> > V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> > matter, and the kernel doesn't expose it. We may or may not consider
> > that as a shortcoming of the V4L2 control API, but in any case it's the
> > situation we have today.
> > 
> >> I wonder if Laurent meant digital gain.
> > 
> > No, I meant analog. It applies to digital gain too though.
> > 
> >> Those are often Q numbers. The practice there has been that the default
> >> value yields gain of 1.
> >>
> >> There are probably many other examples in controls where something being
> >> controlled isn't actually an integer while integer controls are still being
> >> used for the purpose.
> > 
> > A good summary of my opinion :-)
> 
> And that works fine as long as userspace doesn't need to know what the value
> actually means.
> 
> That's not the case here. The control is really a fractional Hz value:
> 
> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
> +Sets the offset from the audio source sample 

Re: Fbdev issue after the drm updates 'drm-next-2023-10-31-1'

[Geert Uytterhoeven]

Hi Christian,

On Sun, Nov 12, 2023 at 3:23 PM Christian Zigotzky
 wrote:
> On 07 November 2023 at 09:36 am, Christian Zigotzky wrote:
> > I have found out that fbdev no longer works with virtio-gpu-pci and
> > virtio-vga. It is not a problem with the penguin logos.
> >
> > Could you please check fbdev in QEMU virtual machines with
> > virtio-gpu-pci and virtio-vga graphics?
>
> > On 02 November 2023 at 03:45 pm, Christian Zigotzky wrote:
> >> There is a fbdev issue with the virtio-gpu-pci and virtio-vga. (The
> >> penguins are not displayed at boot time)
> >>
> >> Error message:  [0.889302] virtio-pci :00:02.0: [drm] *ERROR*
> >> fbdev: Failed to setup generic emulation (ret=-2)
> >>
> >> The kernel 6.6 final doesn't have this issue.
> >>
> >> Please check the fbdev changes in the drm updates
> >> 'drm-next-2023-10-31-1'.

Thanks for your report!

I can confirm there is no graphics output with m68k/virt, and
bisected this to my own commit 6ae2ff23aa43a0c4 ("drm/client: Convert
drm_client_buffer_addfb() to drm_mode_addfb2()"), ouch...

It turns out the old call to drm_mode_addfb() caused a translation
from a fourcc to a bpp/depth pair to a _different_ fourcc, due to the
quirk processing in drm_driver_legacy_fb_format().
I.e. on m68k/virt, the original requested format was XR24, which was
translated to BX24. The former doesn't work, the latter works.

The following (gmail-whitespace-damaged) patch fixed the issue for me:

--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -400,6 +400,16 @@ static int drm_client_buffer_addfb(struct
drm_client_buffer *buffer,

fb_req.width = width;
fb_req.height = height;
+   if (client->dev->mode_config.quirk_addfb_prefer_host_byte_order) {
+   if (format == DRM_FORMAT_XRGB)
+   format = DRM_FORMAT_HOST_XRGB;
+   if (format == DRM_FORMAT_ARGB)
+   format = DRM_FORMAT_HOST_ARGB;
+   if (format == DRM_FORMAT_RGB565)
+   format = DRM_FORMAT_HOST_RGB565;
+   if (format == DRM_FORMAT_XRGB1555)
+   format = DRM_FORMAT_HOST_XRGB1555;
+   }
fb_req.pixel_format = format;
fb_req.handles[0] = handle;
fb_req.pitches[0] = buffer->pitch;

However, I don't think we want to sprinkle more of these
translations around... So perhaps we should (re)add a call to
drm_driver_legacy_fb_format() to drm_client_buffer_addfb()?

Second, as I doubt you are using a big-endian system, you are probably
running into a slightly different issue.

Oh wait, you did CC linuxppc-dev, so perhaps you are running on a
big-endian machine?

If not, please add

pr_info("%s: format = %p4cc\n", __func__, &format);

to drivers/gpu/drm/drm_client.c:drm_client_buffer_addfb(), and,
after reverting commit 6ae2ff23aa43a0c4, add

pr_info("%s: bpp %u/depth %u => r.pixel_format = %p4cc\n",
__func__, or->bpp, or->depth, &r.pixel_format);

to drivers/gpu/drm/drm_framebuffer.c:drm_mode_addfb(), so we know the
translation in your case?

Thanks!

Gr{oetje,eeting}s,

Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 3/3] PCI: dwc: Add dw_pcie_ep_{read,write}_dbi[2] helpers

[Serge Semin]

On Mon, Nov 13, 2023 at 10:33:00AM +0900, Yoshihiro Shimoda wrote:
> The current code calculated some dbi[2] registers' offset by calling
> dw_pcie_ep_get_dbi[2]_offset() in each function. To improve code
> readability, add dw_pcie_ep_{read,write}_dbi[2} and some data-width
> related helpers.

Thanks for submitting this cleanup patch. That's exactly what I meant
here
https://lore.kernel.org/linux-pci/j4g4ijnxd7qyacszlwyi3tdztkw2nmnjwyhdqf2l2yj3h2mvje@iqsrqiodqbhq/
and Mani later here
https://lore.kernel.org/linux-pci/20230728023444.GA4433@thinkpad/

Please note a few nitpicks below.

> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 230 ++
>  1 file changed, 129 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c 
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 1100671db887..dcbed49c9613 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -65,24 +65,89 @@ static unsigned int dw_pcie_ep_get_dbi2_offset(struct 
> dw_pcie_ep *ep, u8 func_no
>   return dbi2_offset;
>  }
>  
> +static u32 dw_pcie_ep_read_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg,
> +size_t size)
> +{
> + unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> + return dw_pcie_read_dbi(pci, offset + reg, size);
> +}
> +
> +static void dw_pcie_ep_write_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg,
> +  size_t size, u32 val)
> +{
> + unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> + dw_pcie_write_dbi(pci, offset + reg, size, val);
> +}
> +
> +static void dw_pcie_ep_write_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 reg,
> +   size_t size, u32 val)
> +{
> + unsigned int offset = dw_pcie_ep_get_dbi2_offset(ep, func_no);
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> + dw_pcie_write_dbi2(pci, offset + reg, size, val);
> +}
> +
> +static inline void dw_pcie_ep_writel_dbi(struct dw_pcie_ep *ep, u8 func_no,
> +  u32 reg, u32 val)
> +{
> + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x4, val);
> +}
> +
> +static inline u32 dw_pcie_ep_readl_dbi(struct dw_pcie_ep *ep, u8 func_no,
> +u32 reg)
> +{
> + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x4);
> +}
> +
> +static inline void dw_pcie_ep_writew_dbi(struct dw_pcie_ep *ep, u8 func_no,
> +  u32 reg, u16 val)
> +{
> + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x2, val);
> +}
> +
> +static inline u16 dw_pcie_ep_readw_dbi(struct dw_pcie_ep *ep, u8 func_no,
> +u32 reg)
> +{
> + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x2);
> +}
> +
> +static inline void dw_pcie_ep_writeb_dbi(struct dw_pcie_ep *ep, u8 func_no,
> +  u32 reg, u8 val)
> +{
> + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x1, val);
> +}
> +
> +static inline u8 dw_pcie_ep_readb_dbi(struct dw_pcie_ep *ep, u8 func_no,
> +   u32 reg)
> +{
> + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x1);
> +}
> +
> +static inline void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 func_no,
> +   u32 reg, u32 val)
> +{
> + dw_pcie_ep_write_dbi2(ep, func_no, reg, 0x4, val);
> +}
> +

I am not sure whether the methods above are supposed to be defined
here instead of being moved to the "pcie-designware.h" header file
together with dw_pcie_ep_get_dbi2_offset() and
dw_pcie_ep_get_dbi_offset(). The later place seems more suitable
seeing the accessors are generic, look similar to the
dw_pcie_{write,read}_dbi{,2}() functions and might be useful in the
platform drivers. On the other hand no LLDDs would have used it
currently. So I'll leave this as a food for thoughts for the driver
and subsystem maintainers.

>  static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
>  enum pci_barno bar, int flags)
>  {
> - unsigned int dbi_offset, dbi2_offset;
>   struct dw_pcie_ep *ep = &pci->ep;
>   u32 reg, reg_dbi2;
>  
> - dbi_offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
> - dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no);
> -
> - reg = dbi_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
> - reg_dbi2 = dbi2_offset + PCI_BASE_ADDRESS_0 + (4 * bar);

> + reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> + reg_dbi2 = PCI_BASE_ADDRESS_0 + (4 * bar);

Semantics of the both variables is identical, could you please drop
"reg_dbi2" and just use the "reg" variable instead here? You must have
just missed it because a similar change is done in the rest of the
places in this patch.

>   dw_pcie_dbi_

Re: [PATCH 0/3] PCI: dwc: Improve code readability

[Krzysztof Wilczyński]

Hello,

[...]
> > > Now, while you are looking at things, can you also take care about the 
> > > following:
> > >
> > >   drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to 
> > > smaller integer type 'enum dw_pcie_device_mode'
> > from 'const void *' [-Wvoid-pointer-to-enum-cast]
> 
> Thank you for the report!
> 
> > > This requires adding structs for each data member of the of_device_id 
> > > type.
> > 
> > That sounds like overkill to me.
> > An intermediate cast to uintptr_t should fix the issue as well.
> 
> I confirmed that the uintptr_t fixed the issue.

We declined a similar fix in the past[1] ...

> I also think that adding a new struct with the mode is overkill.

... with the hopes that a driver could drop the switch statements in place
of using the other pattern.  Also, to be consistent with other drivers that
do this already.

> So, I would like to fix the issue by using the cast of uintptr_t.

Sure.  I appreciate that this would be more work.  When you send your
patch, can you include an update to the iproc driver (and credit the
original author from [1])?  I would appreciate it.

1. https://lore.kernel.org/linux-pci/20230814230008.GA196797@bhelgaas/

Krzysztof

Re: [PATCH v14 00/34] KVM: guest_memfd() and per-page attributes

[Paolo Bonzini]

On 11/5/23 17:30, Paolo Bonzini wrote:

The "development cycle" for this version is going to be very short;
ideally, next week I will merge it as is in kvm/next, taking this through
the KVM tree for 6.8 immediately after the end of the merge window.
The series is still based on 6.6 (plus KVM changes for 6.7) so it
will require a small fixup for changes to get_file_rcu() introduced in
6.7 by commit 0ede61d8589c ("file: convert to SLAB_TYPESAFE_BY_RCU").
The fixup will be done as part of the merge commit, and most of the text
above will become the commit message for the merge.


The changes from review are small enough and entirely in tests, so
I went ahead and pushed it to kvm/next, together with "selftests: kvm/s390x: use 
vm_create_barebones()" which also fixed testcase failures (similar to the 
aarch64/page_fault_test.c hunk below).

The guestmemfd branch on kvm.git was force-pushed, and can be used for further
development if you don't want to run 6.7-rc1 for whatever reason.

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 38882263278d..926241e23aeb 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -1359,7 +1359,6 @@ yet and must be cleared on entry.
__u64 guest_phys_addr;
__u64 memory_size; /* bytes */
__u64 userspace_addr; /* start of the userspace allocated memory */
-   __u64 pad[16];
   };
 
   /* for kvm_userspace_memory_region::flags */

diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c 
b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
index eb4217b7c768..08a5ca5bed56 100644
--- a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
+++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
@@ -705,7 +705,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 
 	print_test_banner(mode, p);
 
-	vm = vm_create(mode);

+   vm = vm_create(VM_SHAPE(mode));
setup_memslots(vm, p);
kvm_vm_elf_load(vm, program_invocation_name);
setup_ucall(vm);
diff --git a/tools/testing/selftests/kvm/guest_memfd_test.c 
b/tools/testing/selftests/kvm/guest_memfd_test.c
index ea0ae7e25330..fd389663c49b 100644
--- a/tools/testing/selftests/kvm/guest_memfd_test.c
+++ b/tools/testing/selftests/kvm/guest_memfd_test.c
@@ -6,14 +6,6 @@
  */
 
 #define _GNU_SOURCE

-#include "test_util.h"
-#include "kvm_util_base.h"
-#include 
-#include 
-#include 
-#include 
-#include 
-
 #include 
 #include 
 #include 
@@ -21,6 +13,15 @@
 #include 
 #include 
 
+#include 

+#include 
+#include 
+#include 
+#include 
+
+#include "test_util.h"
+#include "kvm_util_base.h"
+
 static void test_file_read_write(int fd)
 {
char buf[64];
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h 
b/tools/testing/selftests/kvm/include/kvm_util_base.h
index e4d2cd9218b2..1b58f943562f 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -819,6 +819,7 @@ static inline struct kvm_vm *vm_create_barebones(void)
return vm_create(VM_SHAPE_DEFAULT);
 }
 
+#ifdef __x86_64__

 static inline struct kvm_vm *vm_create_barebones_protected_vm(void)
 {
const struct vm_shape shape = {
@@ -828,6 +829,7 @@ static inline struct kvm_vm 
*vm_create_barebones_protected_vm(void)
 
 	return vm_create(shape);

 }
+#endif
 
 static inline struct kvm_vm *vm_create(uint32_t nr_runnable_vcpus)

 {
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c 
b/tools/testing/selftests/kvm/lib/kvm_util.c
index d05d95cc3693..9b29cbf49476 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1214,7 +1214,7 @@ void vm_guest_mem_fallocate(struct kvm_vm *vm, uint64_t 
base, uint64_t size,
TEST_ASSERT(region && region->region.flags & 
KVM_MEM_GUEST_MEMFD,
"Private memory region not found for GPA 0x%lx", 
gpa);
 
-		offset = (gpa - region->region.guest_phys_addr);

+   offset = gpa - region->region.guest_phys_addr;
fd_offset = region->region.guest_memfd_offset + offset;
len = min_t(uint64_t, end - gpa, region->region.memory_size - 
offset);
 
diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c

index 343e807043e1..1efee1cfcff0 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -433,6 +433,7 @@ static void test_add_max_memory_regions(void)
 }
 
 
+#ifdef __x86_64__

 static void test_invalid_guest_memfd(struct kvm_vm *vm, int memfd,
 size_t offset, const char *msg)
 {
@@ -523,14 +524,13 @@ static void 
test_add_overlapping_private_memory_regions(void)
close(memfd);
kvm_vm_free(vm);
 }
+#endif
 
 int main(int argc, char *argv[])

 {
 #ifdef __x86_64__
int i, loops;
-#endif
 
-#ifdef _

Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Hans Verkuil]

On 13/11/2023 12:43, Laurent Pinchart wrote:
> On Mon, Nov 13, 2023 at 11:28:51AM +, Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
 On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> On 13/11/2023 11:42, Laurent Pinchart wrote:
>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>>> Hi Shengjiu,
>>>
>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
 Fixed point controls are used by the user to configure
 a fixed point value in 64bits, which Q31.32 format.

 Signed-off-by: Shengjiu Wang 
>>>
>>> This patch adds a new control type. This is something that also needs 
>>> to be
>>> tested by v4l2-compliance, and for that we need to add support for this 
>>> to
>>> one of the media test-drivers. The best place for that is the vivid 
>>> driver,
>>> since that has already a bunch of test controls for other control types.
>>>
>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>>
>>> Can you add a patch adding a fixed point test control to vivid?
>>
>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
>> relate more to units than control types. We have lots of fixed-point
>> values in controls already, using the 32-bit and 64-bit integer control
>> types. They use various locations for the decimal point, depending on
>> the control. If we want to make this more explicit to users, we should
>> work on adding unit support to the V4L2 controls.
>
> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.

 It's not a unit, but I think it's related to units. My point is that,
 without units support, I don't see why we need a formal definition of
 fixed-point types, and why this series couldn't just use
 VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
 values as they see fit.
>>>
>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> 
> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> 
>>> is always interpreted as a 64 bit integer and nothing else. As it should.
> 
> The most common case for control handling in drivers is taking the
> integer value and converting it to a register value, using
> device-specific encoding of the register value. It can be a fixed-point
> format or something else, depending on the device. My point is that
> drivers routinely convert a "plain" integer to something else, and that
> has never been considered as a cause of concern. I don't see why it
> would be different in this series.
> 
>>> And while we do not have support for units (other than the documentation),
>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
>>>
> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> only shows a single driver specific control (dw100.rst).
>
> I'm not aware of other controls in mainline that use fixed point.

 The analog gain control for sensors for instance.
>>>
>>> Not really. The documentation is super vague:
>>>
>>> V4L2_CID_ANALOGUE_GAIN (integer)
>>>
>>> Analogue gain is gain affecting all colour components in the pixel 
>>> matrix. The
>>> gain operation is performed in the analogue domain before A/D 
>>> conversion.
>>>
>>> And the integer is just a range. Internally it might map to some fixed
>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
> 
> It's hidden so well that libcamera has a database of the sensor it
> supports, with formulas to map a real gain value to the
> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> matter, and the kernel doesn't expose it. We may or may not consider
> that as a shortcoming of the V4L2 control API, but in any case it's the
> situation we have today.
> 
>> I wonder if Laurent meant digital gain.
> 
> No, I meant analog. It applies to digital gain too though.
> 
>> Those are often Q numbers. The practice there has been that the default
>> value yields gain of 1.
>>
>> There are probably many other examples in controls where something being
>> controlled isn't actually an integer while integer controls are still being
>> used for the purpose.
> 
> A good summary of my opinion :-)

And that works fine as long as userspace doesn't need to know what the value
actually means.

That's not the case here. The control is really a fractional Hz value:

+``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
+Sets the offset from the audio source sample rate, unit is Hz.
+The offset compensates for any clock drift. The actual source audio sample
+rate is the ideal source audio sample rate from
+``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.

> 
>> Instead of this patch,

RE: [PATCH 1/3] PCI: dwc: Rename to .init in struct dw_pcie_ep_ops

[Yoshihiro Shimoda]

Hi Serge,

> From: Serge Semin, Sent: Monday, November 13, 2023 7:15 PM
> 
> Hi Yoshihiro.
> 
> On Mon, Nov 13, 2023 at 10:32:58AM +0900, Yoshihiro Shimoda wrote:
> > Since the name of dw_pcie_ep_ops indicates that it's for ep obviously,
> > rename a member .ep_init to .init.
> 
> Thanks for the series. This change particularly looks good. But since
> you are fixing the redundant prefixes anyway, could you also fix the
> dw_pcie_host_ops structure too (drop host_ prefixes from the
> .host_init() and .host_deinit() fields)? The change was discussed a
> while ago here
> https://lore.kernel.org/linux-pci/20230802104049.GB57374@thinkpad/
> 
> It's better to be done in the framework of a separate patch released
> within this series.

Thank you for reminding me about the discussion. I'll add such a patch in v2.

Best regards,
Yoshihiro Shimoda

> -Serge(y)
> 
> >
> > Signed-off-by: Yoshihiro Shimoda 
> > ---
> >  drivers/pci/controller/dwc/pci-dra7xx.c   | 2 +-
> >  drivers/pci/controller/dwc/pci-imx6.c | 2 +-
> >  drivers/pci/controller/dwc/pci-keystone.c | 2 +-
> >  drivers/pci/controller/dwc/pci-layerscape-ep.c| 2 +-
> >  drivers/pci/controller/dwc/pcie-artpec6.c | 2 +-
> >  drivers/pci/controller/dwc/pcie-designware-ep.c   | 4 ++--
> >  drivers/pci/controller/dwc/pcie-designware-plat.c | 2 +-
> >  drivers/pci/controller/dwc/pcie-designware.h  | 2 +-
> >  drivers/pci/controller/dwc/pcie-keembay.c | 2 +-
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 2 +-
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c   | 2 +-
> >  drivers/pci/controller/dwc/pcie-uniphier-ep.c | 2 +-
> >  12 files changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c 
> > b/drivers/pci/controller/dwc/pci-dra7xx.c
> > index b445ffe95e3f..f9182cd6fe67 100644
> > --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> > @@ -436,7 +436,7 @@ dra7xx_pcie_get_features(struct dw_pcie_ep *ep)
> >  }
> >
> >  static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > -   .ep_init = dra7xx_pcie_ep_init,
> > +   .init = dra7xx_pcie_ep_init,
> > .raise_irq = dra7xx_pcie_raise_irq,
> > .get_features = dra7xx_pcie_get_features,
> >  };
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c 
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 74703362aeec..737d4d90fef2 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1093,7 +1093,7 @@ imx6_pcie_ep_get_features(struct dw_pcie_ep *ep)
> >  }
> >
> >  static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > -   .ep_init = imx6_pcie_ep_init,
> > +   .init = imx6_pcie_ep_init,
> > .raise_irq = imx6_pcie_ep_raise_irq,
> > .get_features = imx6_pcie_ep_get_features,
> >  };
> > diff --git a/drivers/pci/controller/dwc/pci-keystone.c 
> > b/drivers/pci/controller/dwc/pci-keystone.c
> > index 0def919f89fa..655c7307eb88 100644
> > --- a/drivers/pci/controller/dwc/pci-keystone.c
> > +++ b/drivers/pci/controller/dwc/pci-keystone.c
> > @@ -944,7 +944,7 @@ ks_pcie_am654_get_features(struct dw_pcie_ep *ep)
> >  }
> >
> >  static const struct dw_pcie_ep_ops ks_pcie_am654_ep_ops = {
> > -   .ep_init = ks_pcie_am654_ep_init,
> > +   .init = ks_pcie_am654_ep_init,
> > .raise_irq = ks_pcie_am654_raise_irq,
> > .get_features = &ks_pcie_am654_get_features,
> >  };
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c 
> > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > index 3d3c50ef4b6f..4e4b687ef508 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > @@ -195,7 +195,7 @@ static unsigned int ls_pcie_ep_func_conf_select(struct 
> > dw_pcie_ep *ep,
> >  }
> >
> >  static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> > -   .ep_init = ls_pcie_ep_init,
> > +   .init = ls_pcie_ep_init,
> > .raise_irq = ls_pcie_ep_raise_irq,
> > .get_features = ls_pcie_ep_get_features,
> > .func_conf_select = ls_pcie_ep_func_conf_select,
> > diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c 
> > b/drivers/pci/controller/dwc/pcie-artpec6.c
> > index 9b572a2b2c9a..27ff425c0698 100644
> > --- a/drivers/pci/controller/dwc/pcie-artpec6.c
> > +++ b/drivers/pci/controller/dwc/pcie-artpec6.c
> > @@ -370,7 +370,7 @@ static int artpec6_pcie_raise_irq(struct dw_pcie_ep 
> > *ep, u8 func_no,
> >  }
> >
> >  static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > -   .ep_init = artpec6_pcie_ep_init,
> > +   .init = artpec6_pcie_ep_init,
> > .raise_irq = artpec6_pcie_raise_irq,
> >  };
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c 
> > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index f6207989fc6a..ea99a97ce504 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -794,8 +794,8 @@ int dw_pcie_ep_init(struct dw_pc

Re: [RFC PATCH] powerpc/book3s/hash: Drop _PAGE_PRIVILEGED from PAGE_NONE

[Nicholas Piggin]

On Mon Nov 13, 2023 at 8:45 PM AEST, Aneesh Kumar K V wrote:
> On 11/13/23 3:46 PM, Nicholas Piggin wrote:
> > On Thu Nov 2, 2023 at 11:23 PM AEST, Aneesh Kumar K.V wrote:
> >> There used to be a dependency on _PAGE_PRIVILEGED with pte_savedwrite.
> >> But that got dropped by
> >> commit 6a56ccbcf6c6 ("mm/autonuma: use can_change_(pte|pmd)_writable() to 
> >> replace savedwrite")
> >>
> >> With this change numa fault pte (pte_protnone()) gets mapped as regular 
> >> user pte
> >> with RWX cleared (no-access).
> > 
> > You mean "that" above change (not *this* change), right?
> > 
>
> With the change in this patch numa fault pte will not have _PAGE_PRIVILEGED 
> set because 
> PAGE_NONE now maps to just _PAGE_BASE

Right, I guess I was confused because I missed the pgtable-mask.h move.

> >> This also remove pte_user() from
> >> book3s/64.
> > 
> > Nice cleanup. That was an annoying hack.
> > 
> >> pte_access_permitted() now checks for _PAGE_EXEC because we now support
> >> EXECONLY mappings.
> > 
> > AFAIKS pte_exec() is not required, GUP is really only for read or
> > write access. It should be a separate patch if you think it's needed.
> > 
>
> I have a v2 dropping that based on 
> https://lore.kernel.org/linux-mm/87bkc1oe8c@linux.ibm.com 
> I kept pte_user with pte_access_permitted being the only user. I can open 
> code that
> if needed. 

I don't mind keeping pte_user() and the check in pte_access_permitted().

> >>
> >> Signed-off-by: Aneesh Kumar K.V 
> >> ---
> >>  arch/powerpc/include/asm/book3s/64/pgtable.h | 23 +---
> >>  arch/powerpc/mm/book3s64/hash_utils.c| 17 +++
> >>  2 files changed, 23 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h 
> >> b/arch/powerpc/include/asm/book3s/64/pgtable.h
> >> index cb77eddca54b..7c7de7b56df0 100644
> >> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> >> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> >> @@ -17,12 +17,6 @@
> >>  #define _PAGE_EXEC0x1 /* execute permission */
> >>  #define _PAGE_WRITE   0x2 /* write access allowed */
> >>  #define _PAGE_READ0x4 /* read access allowed */
> >> -#define _PAGE_NA  _PAGE_PRIVILEGED
> >> -#define _PAGE_NAX _PAGE_EXEC
> >> -#define _PAGE_RO  _PAGE_READ
> >> -#define _PAGE_ROX (_PAGE_READ | _PAGE_EXEC)
> >> -#define _PAGE_RW  (_PAGE_READ | _PAGE_WRITE)
> >> -#define _PAGE_RWX (_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC)
> >>  #define _PAGE_PRIVILEGED  0x8 /* kernel access only */
> >>  #define _PAGE_SAO 0x00010 /* Strong access order */
> >>  #define _PAGE_NON_IDEMPOTENT  0x00020 /* non idempotent memory */
> > 
> > Did you leave PAGE_NONE as _PAGE_BASE | _PAGE_PRIVILEGED below?
> > Shouldn't that be changed too? Then this patch is not only hash
> > but also radix.
> > 
>
> A recent patch moved PAGE_NONE to pgtable-mask.h 
> a5a08dc90f4513d1a78582ec24b687fad01cc843

Thanks I did miss it.

>
> > Why is the hash change required? Previously PAGE_NONE relied on
> > privileged bit to prevent access, now you need to handle a PTE
> > without that bit? In that case could that be patch 1, then the
> > rest patch 2?
> > 
>
> Looking at older kernel, I guess check_pte_access used _PAGE_PRIVILEGED 
> as a way to prevent access to PAGE_NONE ptes. We now depend on
> _PAGE_READ
>
> > __pte_flags_need_flush() should be updated after this too,
> > basically revert commit 1abce0580b894.
> > 
>
> Will update the patch to include the revert.
>
> >> @@ -119,9 +113,9 @@
> >>  /*
> >>   * user access blocked by key
> >>   */
> >> -#define _PAGE_KERNEL_RW   (_PAGE_PRIVILEGED | _PAGE_RW | 
> >> _PAGE_DIRTY)
> >>  #define _PAGE_KERNEL_RO(_PAGE_PRIVILEGED | _PAGE_READ)
> >>  #define _PAGE_KERNEL_ROX   (_PAGE_PRIVILEGED | _PAGE_READ | _PAGE_EXEC)
> >> +#define _PAGE_KERNEL_RW   (_PAGE_PRIVILEGED | _PAGE_RW | 
> >> _PAGE_DIRTY)
> >>  #define _PAGE_KERNEL_RWX  (_PAGE_PRIVILEGED | _PAGE_DIRTY | _PAGE_RW | 
> >> _PAGE_EXEC)
> >>  /*
> >>   * _PAGE_CHG_MASK masks of bits that are to be preserved across
> > 
> > No need to reorder defines.
> > 
>
> ok
>
>
> > Thanks,
> > Nick
> > 
> >> @@ -523,19 +517,14 @@ static inline bool arch_pte_access_permitted(u64 
> >> pte, bool write, bool execute)
> >>  }
> >>  #endif /* CONFIG_PPC_MEM_KEYS */
> >>  
> >> -static inline bool pte_user(pte_t pte)
> >> -{
> >> -  return !(pte_raw(pte) & cpu_to_be64(_PAGE_PRIVILEGED));
> >> -}
> >> -
> >>  #define pte_access_permitted pte_access_permitted
> >>  static inline bool pte_access_permitted(pte_t pte, bool write)
> >>  {
> >> -  /*
> >> -   * _PAGE_READ is needed for any access and will be
> >> -   * cleared for PROT_NONE
> >> -   */
> >> -  if (!pte_present(pte) || !pte_user(pte) || !pte_read(pte))
> >> +
> >> +  if (!pte_present(pte))
> >> +  return false;
> >> +
> >> +  if (!(pte_read(pte) || pte_exec(pte)))
> >>  

RE: [PATCH 0/3] PCI: dwc: Improve code readability

[Yoshihiro Shimoda]

Hi Krzysztof-san, Geert-san,

> From: Geert Uytterhoeven, Sent: Monday, November 13, 2023 8:07 PM
> 
> Hi Krzysztof,
> 
> On Mon, Nov 13, 2023 at 11:09 AM Krzysztof Wilczyński  wrote:
> > > This patch series is based on the latest pci.git / next branch.
> > [...]
> >
> > Thank you for following up to tidy things up!  Much appreciated.
> >
> > Now, while you are looking at things, can you also take care about the 
> > following:
> >
> >   drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to 
> > smaller integer type 'enum dw_pcie_device_mode'
> from 'const void *' [-Wvoid-pointer-to-enum-cast]

Thank you for the report!

> > This requires adding structs for each data member of the of_device_id type.
> 
> That sounds like overkill to me.
> An intermediate cast to uintptr_t should fix the issue as well.

I confirmed that the uintptr_t fixed the issue.
I also think that adding a new struct with the mode is overkill.
So, I would like to fix the issue by using the cast of uintptr_t.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Laurent Pinchart]

On Mon, Nov 13, 2023 at 11:28:51AM +, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> > On 13/11/2023 12:07, Laurent Pinchart wrote:
> > > On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> > >> On 13/11/2023 11:42, Laurent Pinchart wrote:
> > >>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >  Hi Shengjiu,
> > 
> >  On 10/11/2023 06:48, Shengjiu Wang wrote:
> > > Fixed point controls are used by the user to configure
> > > a fixed point value in 64bits, which Q31.32 format.
> > >
> > > Signed-off-by: Shengjiu Wang 
> > 
> >  This patch adds a new control type. This is something that also needs 
> >  to be
> >  tested by v4l2-compliance, and for that we need to add support for 
> >  this to
> >  one of the media test-drivers. The best place for that is the vivid 
> >  driver,
> >  since that has already a bunch of test controls for other control 
> >  types.
> > 
> >  See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> > 
> >  Can you add a patch adding a fixed point test control to vivid?
> > >>>
> > >>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> > >>> relate more to units than control types. We have lots of fixed-point
> > >>> values in controls already, using the 32-bit and 64-bit integer control
> > >>> types. They use various locations for the decimal point, depending on
> > >>> the control. If we want to make this more explicit to users, we should
> > >>> work on adding unit support to the V4L2 controls.
> > >>
> > >> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> > > 
> > > It's not a unit, but I think it's related to units. My point is that,
> > > without units support, I don't see why we need a formal definition of
> > > fixed-point types, and why this series couldn't just use
> > > VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> > > values as they see fit.
> > 
> > They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> > (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it

Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)

> > is always interpreted as a 64 bit integer and nothing else. As it should.

The most common case for control handling in drivers is taking the
integer value and converting it to a register value, using
device-specific encoding of the register value. It can be a fixed-point
format or something else, depending on the device. My point is that
drivers routinely convert a "plain" integer to something else, and that
has never been considered as a cause of concern. I don't see why it
would be different in this series.

> > And while we do not have support for units (other than the documentation),
> > we do have type support in the form of V4L2_CTRL_TYPE_*.
> > 
> > >> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> > >> only shows a single driver specific control (dw100.rst).
> > >>
> > >> I'm not aware of other controls in mainline that use fixed point.
> > > 
> > > The analog gain control for sensors for instance.
> > 
> > Not really. The documentation is super vague:
> > 
> > V4L2_CID_ANALOGUE_GAIN (integer)
> > 
> > Analogue gain is gain affecting all colour components in the pixel 
> > matrix. The
> > gain operation is performed in the analogue domain before A/D 
> > conversion.
> > 
> > And the integer is just a range. Internally it might map to some fixed
> > point value, but userspace won't see that, it's hidden in the driver AFAICT.

It's hidden so well that libcamera has a database of the sensor it
supports, with formulas to map a real gain value to the
V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
matter, and the kernel doesn't expose it. We may or may not consider
that as a shortcoming of the V4L2 control API, but in any case it's the
situation we have today.

> I wonder if Laurent meant digital gain.

No, I meant analog. It applies to digital gain too though.

> Those are often Q numbers. The practice there has been that the default
> value yields gain of 1.
> 
> There are probably many other examples in controls where something being
> controlled isn't actually an integer while integer controls are still being
> used for the purpose.

A good summary of my opinion :-)

> Instead of this patch, I'd prefer to have a way to express the meaning of
> the control value, be it a Q number or something else, and do that
> independently of the type of the control.

Agreed.

> > In the case of this particular series the control type is really a fixed 
> > point
> > value with a documented unit (Hz). It really is not something you want to
> > use type INTEGER64 for.
> > 
> > >> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> > >> min/max/step you can easily map that to just about any QN.M format where
> > >> N <= 31

Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Sakari Ailus]

Hi Hans,

On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> On 13/11/2023 12:07, Laurent Pinchart wrote:
> > On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> >> On 13/11/2023 11:42, Laurent Pinchart wrote:
> >>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>  Hi Shengjiu,
> 
>  On 10/11/2023 06:48, Shengjiu Wang wrote:
> > Fixed point controls are used by the user to configure
> > a fixed point value in 64bits, which Q31.32 format.
> >
> > Signed-off-by: Shengjiu Wang 
> 
>  This patch adds a new control type. This is something that also needs to 
>  be
>  tested by v4l2-compliance, and for that we need to add support for this 
>  to
>  one of the media test-drivers. The best place for that is the vivid 
>  driver,
>  since that has already a bunch of test controls for other control types.
> 
>  See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> 
>  Can you add a patch adding a fixed point test control to vivid?
> >>>
> >>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> >>> relate more to units than control types. We have lots of fixed-point
> >>> values in controls already, using the 32-bit and 64-bit integer control
> >>> types. They use various locations for the decimal point, depending on
> >>> the control. If we want to make this more explicit to users, we should
> >>> work on adding unit support to the V4L2 controls.
> >>
> >> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> > 
> > It's not a unit, but I think it's related to units. My point is that,
> > without units support, I don't see why we need a formal definition of
> > fixed-point types, and why this series couldn't just use
> > VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> > values as they see fit.
> 
> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> is always interpreted as a 64 bit integer and nothing else. As it should.
> 
> And while we do not have support for units (other than the documentation),
> we do have type support in the form of V4L2_CTRL_TYPE_*.
> 
> > 
> >> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> >> only shows a single driver specific control (dw100.rst).
> >>
> >> I'm not aware of other controls in mainline that use fixed point.
> > 
> > The analog gain control for sensors for instance.
> 
> Not really. The documentation is super vague:
> 
> V4L2_CID_ANALOGUE_GAIN (integer)
> 
>   Analogue gain is gain affecting all colour components in the pixel 
> matrix. The
>   gain operation is performed in the analogue domain before A/D 
> conversion.
> 
> And the integer is just a range. Internally it might map to some fixed
> point value, but userspace won't see that, it's hidden in the driver AFAICT.

I wonder if Laurent meant digital gain.

Those are often Q numbers. The practice there has been that the default
value yields gain of 1.

There are probably many other examples in controls where something being
controlled isn't actually an integer while integer controls are still being
used for the purpose.

Instead of this patch, I'd prefer to have a way to express the meaning of
the control value, be it a Q number or something else, and do that
independently of the type of the control.

> 
> In the case of this particular series the control type is really a fixed point
> value with a documented unit (Hz). It really is not something you want to
> use type INTEGER64 for.
> 
> > 
> >> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> >> min/max/step you can easily map that to just about any QN.M format where
> >> N <= 31 and M <= 32.
> >>
> >> In the case of dw100 it is a bit different in that it is quite specialized
> >> and it had to fit in 16 bits.

-- 
Regards,

Sakari Ailus

Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Hans Verkuil]

On 13/11/2023 12:07, Laurent Pinchart wrote:
> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
>> On 13/11/2023 11:42, Laurent Pinchart wrote:
>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
 Hi Shengjiu,

 On 10/11/2023 06:48, Shengjiu Wang wrote:
> Fixed point controls are used by the user to configure
> a fixed point value in 64bits, which Q31.32 format.
>
> Signed-off-by: Shengjiu Wang 

 This patch adds a new control type. This is something that also needs to be
 tested by v4l2-compliance, and for that we need to add support for this to
 one of the media test-drivers. The best place for that is the vivid driver,
 since that has already a bunch of test controls for other control types.

 See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.

 Can you add a patch adding a fixed point test control to vivid?
>>>
>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
>>> relate more to units than control types. We have lots of fixed-point
>>> values in controls already, using the 32-bit and 64-bit integer control
>>> types. They use various locations for the decimal point, depending on
>>> the control. If we want to make this more explicit to users, we should
>>> work on adding unit support to the V4L2 controls.
>>
>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> 
> It's not a unit, but I think it's related to units. My point is that,
> without units support, I don't see why we need a formal definition of
> fixed-point types, and why this series couldn't just use
> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> values as they see fit.

They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
(I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
is always interpreted as a 64 bit integer and nothing else. As it should.

And while we do not have support for units (other than the documentation),
we do have type support in the form of V4L2_CTRL_TYPE_*.

> 
>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
>> only shows a single driver specific control (dw100.rst).
>>
>> I'm not aware of other controls in mainline that use fixed point.
> 
> The analog gain control for sensors for instance.

Not really. The documentation is super vague:

V4L2_CID_ANALOGUE_GAIN (integer)

Analogue gain is gain affecting all colour components in the pixel 
matrix. The
gain operation is performed in the analogue domain before A/D 
conversion.

And the integer is just a range. Internally it might map to some fixed
point value, but userspace won't see that, it's hidden in the driver AFAICT.

In the case of this particular series the control type is really a fixed point
value with a documented unit (Hz). It really is not something you want to
use type INTEGER64 for.

> 
>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
>> min/max/step you can easily map that to just about any QN.M format where
>> N <= 31 and M <= 32.
>>
>> In the case of dw100 it is a bit different in that it is quite specialized
>> and it had to fit in 16 bits.

Regards,

Hans

Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Laurent Pinchart]

On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> On 13/11/2023 11:42, Laurent Pinchart wrote:
> > On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >> Hi Shengjiu,
> >>
> >> On 10/11/2023 06:48, Shengjiu Wang wrote:
> >>> Fixed point controls are used by the user to configure
> >>> a fixed point value in 64bits, which Q31.32 format.
> >>>
> >>> Signed-off-by: Shengjiu Wang 
> >>
> >> This patch adds a new control type. This is something that also needs to be
> >> tested by v4l2-compliance, and for that we need to add support for this to
> >> one of the media test-drivers. The best place for that is the vivid driver,
> >> since that has already a bunch of test controls for other control types.
> >>
> >> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> >>
> >> Can you add a patch adding a fixed point test control to vivid?
> > 
> > I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> > relate more to units than control types. We have lots of fixed-point
> > values in controls already, using the 32-bit and 64-bit integer control
> > types. They use various locations for the decimal point, depending on
> > the control. If we want to make this more explicit to users, we should
> > work on adding unit support to the V4L2 controls.
> 
> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.

It's not a unit, but I think it's related to units. My point is that,
without units support, I don't see why we need a formal definition of
fixed-point types, and why this series couldn't just use
VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
values as they see fit.

> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> only shows a single driver specific control (dw100.rst).
> 
> I'm not aware of other controls in mainline that use fixed point.

The analog gain control for sensors for instance.

> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> min/max/step you can easily map that to just about any QN.M format where
> N <= 31 and M <= 32.
> 
> In the case of dw100 it is a bit different in that it is quite specialized
> and it had to fit in 16 bits.
> 
> >>> ---
> >>>  .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++--
> >>>  .../userspace-api/media/v4l/vidioc-queryctrl.rst|  9 -
> >>>  .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
> >>>  drivers/media/v4l2-core/v4l2-ctrls-api.c|  5 -
> >>>  drivers/media/v4l2-core/v4l2-ctrls-core.c   |  2 ++
> >>>  include/uapi/linux/videodev2.h  |  1 +
> >>>  6 files changed, 23 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst 
> >>> b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> index e8475f9fd2cf..e7e5d78dc11e 100644
> >>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> @@ -162,13 +162,13 @@ still cause this situation.
> >>>  * - __s32
> >>>- ``value``
> >>>- New value or current value. Valid if this control is not of type
> >>> - ``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> >>> - not set.
> >>> + ``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> >>> + ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
> >>>  * - __s64
> >>>- ``value64``
> >>>- New value or current value. Valid if this control is of type
> >>> - ``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> >>> - not set.
> >>> + ``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> >>> + ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
> >>>  * - char *
> >>>- ``string``
> >>>- A pointer to a string. Valid if this control is of type
> >>> @@ -193,8 +193,9 @@ still cause this situation.
> >>>  * - __s64 *
> >>>- ``p_s64``
> >>>- A pointer to a matrix control of signed 64-bit values. Valid if
> >>> -this control is of type ``V4L2_CTRL_TYPE_INTEGER64`` and
> >>> -``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is set.
> >>> +this control is of type ``V4L2_CTRL_TYPE_INTEGER64``,
> >>> +``V4L2_CTRL_TYPE_FIXED_POINT`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
> >>> +is set.
> >>>  * - struct :c:type:`v4l2_area` *
> >>>- ``p_area``
> >>>- A pointer to a struct :c:type:`v4l2_area`. Valid if this control 
> >>> is
> >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst 
> >>> b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>> index 4d38acafe8e1..f3995ec57044 100644
> >>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>> @@ -235,7 +235,8 @@ See also the examples in :ref:`control`.
> >>>- ``default_value``
> >>>- The default value of 

Re: [PATCH 0/3] PCI: dwc: Improve code readability

[Geert Uytterhoeven]

Hi Krzysztof,

On Mon, Nov 13, 2023 at 11:09 AM Krzysztof Wilczyński  wrote:
> > This patch series is based on the latest pci.git / next branch.
> [...]
>
> Thank you for following up to tidy things up!  Much appreciated.
>
> Now, while you are looking at things, can you also take care about the 
> following:
>
>   drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to 
> smaller integer type 'enum dw_pcie_device_mode' from 'const void *' 
> [-Wvoid-pointer-to-enum-cast]
>
> This requires adding structs for each data member of the of_device_id type.

That sounds like overkill to me.
An intermediate cast to uintptr_t should fix the issue as well.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Hans Verkuil]

On 13/11/2023 11:42, Laurent Pinchart wrote:
> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>> Hi Shengjiu,
>>
>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>>> Fixed point controls are used by the user to configure
>>> a fixed point value in 64bits, which Q31.32 format.
>>>
>>> Signed-off-by: Shengjiu Wang 
>>
>> This patch adds a new control type. This is something that also needs to be
>> tested by v4l2-compliance, and for that we need to add support for this to
>> one of the media test-drivers. The best place for that is the vivid driver,
>> since that has already a bunch of test controls for other control types.
>>
>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>
>> Can you add a patch adding a fixed point test control to vivid?
> 
> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> relate more to units than control types. We have lots of fixed-point
> values in controls already, using the 32-bit and 64-bit integer control
> types. They use various locations for the decimal point, depending on
> the control. If we want to make this more explicit to users, we should
> work on adding unit support to the V4L2 controls.

"Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.

A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
only shows a single driver specific control (dw100.rst).

I'm not aware of other controls in mainline that use fixed point.

Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
min/max/step you can easily map that to just about any QN.M format where
N <= 31 and M <= 32.

In the case of dw100 it is a bit different in that it is quite specialized
and it had to fit in 16 bits.

Regards,

Hans

> 
>>> ---
>>>  .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++--
>>>  .../userspace-api/media/v4l/vidioc-queryctrl.rst|  9 -
>>>  .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
>>>  drivers/media/v4l2-core/v4l2-ctrls-api.c|  5 -
>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c   |  2 ++
>>>  include/uapi/linux/videodev2.h  |  1 +
>>>  6 files changed, 23 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst 
>>> b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> index e8475f9fd2cf..e7e5d78dc11e 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> @@ -162,13 +162,13 @@ still cause this situation.
>>>  * - __s32
>>>- ``value``
>>>- New value or current value. Valid if this control is not of type
>>> -   ``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
>>> -   not set.
>>> +   ``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
>>> +   ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>>>  * - __s64
>>>- ``value64``
>>>- New value or current value. Valid if this control is of type
>>> -   ``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
>>> -   not set.
>>> +   ``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
>>> +   ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>>>  * - char *
>>>- ``string``
>>>- A pointer to a string. Valid if this control is of type
>>> @@ -193,8 +193,9 @@ still cause this situation.
>>>  * - __s64 *
>>>- ``p_s64``
>>>- A pointer to a matrix control of signed 64-bit values. Valid if
>>> -this control is of type ``V4L2_CTRL_TYPE_INTEGER64`` and
>>> -``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is set.
>>> +this control is of type ``V4L2_CTRL_TYPE_INTEGER64``,
>>> +``V4L2_CTRL_TYPE_FIXED_POINT`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
>>> +is set.
>>>  * - struct :c:type:`v4l2_area` *
>>>- ``p_area``
>>>- A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst 
>>> b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>> index 4d38acafe8e1..f3995ec57044 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>> @@ -235,7 +235,8 @@ See also the examples in :ref:`control`.
>>>- ``default_value``
>>>- The default value of a ``V4L2_CTRL_TYPE_INTEGER``, ``_INTEGER64``,
>>> ``_BOOLEAN``, ``_BITMASK``, ``_MENU``, ``_INTEGER_MENU``, ``_U8``
>>> -   or ``_U16`` control. Not valid for other types of controls.
>>> +   ``_FIXED_POINT`` or ``_U16`` control. Not valid for other types of
>>> +   controls.
>>>  
>>> .. note::
>>>  
>>> @@ -549,6 +550,12 @@ See also the examples in :ref:`control`.
>>>- n/a
>>>- A struct :c:type:`v4l2_ctrl_av1_film_grain`, containing AV1 Film 
>>> Grain
>>>  parameters for stateless video decoders.
>>> +

Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Laurent Pinchart]

On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> Hi Shengjiu,
> 
> On 10/11/2023 06:48, Shengjiu Wang wrote:
> > Fixed point controls are used by the user to configure
> > a fixed point value in 64bits, which Q31.32 format.
> > 
> > Signed-off-by: Shengjiu Wang 
> 
> This patch adds a new control type. This is something that also needs to be
> tested by v4l2-compliance, and for that we need to add support for this to
> one of the media test-drivers. The best place for that is the vivid driver,
> since that has already a bunch of test controls for other control types.
> 
> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> 
> Can you add a patch adding a fixed point test control to vivid?

I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
relate more to units than control types. We have lots of fixed-point
values in controls already, using the 32-bit and 64-bit integer control
types. They use various locations for the decimal point, depending on
the control. If we want to make this more explicit to users, we should
work on adding unit support to the V4L2 controls.

> > ---
> >  .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++--
> >  .../userspace-api/media/v4l/vidioc-queryctrl.rst|  9 -
> >  .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
> >  drivers/media/v4l2-core/v4l2-ctrls-api.c|  5 -
> >  drivers/media/v4l2-core/v4l2-ctrls-core.c   |  2 ++
> >  include/uapi/linux/videodev2.h  |  1 +
> >  6 files changed, 23 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst 
> > b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > index e8475f9fd2cf..e7e5d78dc11e 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > @@ -162,13 +162,13 @@ still cause this situation.
> >  * - __s32
> >- ``value``
> >- New value or current value. Valid if this control is not of type
> > -   ``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> > -   not set.
> > +   ``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> > +   ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
> >  * - __s64
> >- ``value64``
> >- New value or current value. Valid if this control is of type
> > -   ``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> > -   not set.
> > +   ``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> > +   ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
> >  * - char *
> >- ``string``
> >- A pointer to a string. Valid if this control is of type
> > @@ -193,8 +193,9 @@ still cause this situation.
> >  * - __s64 *
> >- ``p_s64``
> >- A pointer to a matrix control of signed 64-bit values. Valid if
> > -this control is of type ``V4L2_CTRL_TYPE_INTEGER64`` and
> > -``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is set.
> > +this control is of type ``V4L2_CTRL_TYPE_INTEGER64``,
> > +``V4L2_CTRL_TYPE_FIXED_POINT`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
> > +is set.
> >  * - struct :c:type:`v4l2_area` *
> >- ``p_area``
> >- A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst 
> > b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > index 4d38acafe8e1..f3995ec57044 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > @@ -235,7 +235,8 @@ See also the examples in :ref:`control`.
> >- ``default_value``
> >- The default value of a ``V4L2_CTRL_TYPE_INTEGER``, ``_INTEGER64``,
> > ``_BOOLEAN``, ``_BITMASK``, ``_MENU``, ``_INTEGER_MENU``, ``_U8``
> > -   or ``_U16`` control. Not valid for other types of controls.
> > +   ``_FIXED_POINT`` or ``_U16`` control. Not valid for other types of
> > +   controls.
> >  
> > .. note::
> >  
> > @@ -549,6 +550,12 @@ See also the examples in :ref:`control`.
> >- n/a
> >- A struct :c:type:`v4l2_ctrl_av1_film_grain`, containing AV1 Film 
> > Grain
> >  parameters for stateless video decoders.
> > +* - ``V4L2_CTRL_TYPE_FIXED_POINT``
> > +  - any
> > +  - any
> > +  - any
> > +  - A 64-bit integer valued control, containing parameter which is
> > +Q31.32 format.
> >  
> >  .. raw:: latex
> >  
> > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions 
> > b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > index e61152bb80d1..2faa5a2015eb 100644
> > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > @@ -167,6 +167,7 @@ replace symbol V4L2_CTRL_TYPE_AV1_SEQUENCE 
> > :c:

Re: [RFC PATCH] powerpc/book3s/hash: Drop _PAGE_PRIVILEGED from PAGE_NONE

[Aneesh Kumar K V]

On 11/13/23 3:46 PM, Nicholas Piggin wrote:
> On Thu Nov 2, 2023 at 11:23 PM AEST, Aneesh Kumar K.V wrote:
>> There used to be a dependency on _PAGE_PRIVILEGED with pte_savedwrite.
>> But that got dropped by
>> commit 6a56ccbcf6c6 ("mm/autonuma: use can_change_(pte|pmd)_writable() to 
>> replace savedwrite")
>>
>> With this change numa fault pte (pte_protnone()) gets mapped as regular user 
>> pte
>> with RWX cleared (no-access).
> 
> You mean "that" above change (not *this* change), right?
> 

With the change in this patch numa fault pte will not have _PAGE_PRIVILEGED set 
because 
PAGE_NONE now maps to just _PAGE_BASE


>> This also remove pte_user() from
>> book3s/64.
> 
> Nice cleanup. That was an annoying hack.
> 
>> pte_access_permitted() now checks for _PAGE_EXEC because we now support
>> EXECONLY mappings.
> 
> AFAIKS pte_exec() is not required, GUP is really only for read or
> write access. It should be a separate patch if you think it's needed.
> 

I have a v2 dropping that based on 
https://lore.kernel.org/linux-mm/87bkc1oe8c@linux.ibm.com 
I kept pte_user with pte_access_permitted being the only user. I can open code 
that
if needed. 

>>
>> Signed-off-by: Aneesh Kumar K.V 
>> ---
>>  arch/powerpc/include/asm/book3s/64/pgtable.h | 23 +---
>>  arch/powerpc/mm/book3s64/hash_utils.c| 17 +++
>>  2 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h 
>> b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> index cb77eddca54b..7c7de7b56df0 100644
>> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> @@ -17,12 +17,6 @@
>>  #define _PAGE_EXEC  0x1 /* execute permission */
>>  #define _PAGE_WRITE 0x2 /* write access allowed */
>>  #define _PAGE_READ  0x4 /* read access allowed */
>> -#define _PAGE_NA_PAGE_PRIVILEGED
>> -#define _PAGE_NAX   _PAGE_EXEC
>> -#define _PAGE_RO_PAGE_READ
>> -#define _PAGE_ROX   (_PAGE_READ | _PAGE_EXEC)
>> -#define _PAGE_RW(_PAGE_READ | _PAGE_WRITE)
>> -#define _PAGE_RWX   (_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC)
>>  #define _PAGE_PRIVILEGED0x8 /* kernel access only */
>>  #define _PAGE_SAO   0x00010 /* Strong access order */
>>  #define _PAGE_NON_IDEMPOTENT0x00020 /* non idempotent memory */
> 
> Did you leave PAGE_NONE as _PAGE_BASE | _PAGE_PRIVILEGED below?
> Shouldn't that be changed too? Then this patch is not only hash
> but also radix.
> 

A recent patch moved PAGE_NONE to pgtable-mask.h 
a5a08dc90f4513d1a78582ec24b687fad01cc843

> Why is the hash change required? Previously PAGE_NONE relied on
> privileged bit to prevent access, now you need to handle a PTE
> without that bit? In that case could that be patch 1, then the
> rest patch 2?
> 

Looking at older kernel, I guess check_pte_access used _PAGE_PRIVILEGED 
as a way to prevent access to PAGE_NONE ptes. We now depend on
_PAGE_READ

> __pte_flags_need_flush() should be updated after this too,
> basically revert commit 1abce0580b894.
> 

Will update the patch to include the revert.

>> @@ -119,9 +113,9 @@
>>  /*
>>   * user access blocked by key
>>   */
>> -#define _PAGE_KERNEL_RW (_PAGE_PRIVILEGED | _PAGE_RW | 
>> _PAGE_DIRTY)
>>  #define _PAGE_KERNEL_RO  (_PAGE_PRIVILEGED | _PAGE_READ)
>>  #define _PAGE_KERNEL_ROX (_PAGE_PRIVILEGED | _PAGE_READ | _PAGE_EXEC)
>> +#define _PAGE_KERNEL_RW (_PAGE_PRIVILEGED | _PAGE_RW | 
>> _PAGE_DIRTY)
>>  #define _PAGE_KERNEL_RWX(_PAGE_PRIVILEGED | _PAGE_DIRTY | _PAGE_RW | 
>> _PAGE_EXEC)
>>  /*
>>   * _PAGE_CHG_MASK masks of bits that are to be preserved across
> 
> No need to reorder defines.
> 

ok


> Thanks,
> Nick
> 
>> @@ -523,19 +517,14 @@ static inline bool arch_pte_access_permitted(u64 pte, 
>> bool write, bool execute)
>>  }
>>  #endif /* CONFIG_PPC_MEM_KEYS */
>>  
>> -static inline bool pte_user(pte_t pte)
>> -{
>> -return !(pte_raw(pte) & cpu_to_be64(_PAGE_PRIVILEGED));
>> -}
>> -
>>  #define pte_access_permitted pte_access_permitted
>>  static inline bool pte_access_permitted(pte_t pte, bool write)
>>  {
>> -/*
>> - * _PAGE_READ is needed for any access and will be
>> - * cleared for PROT_NONE
>> - */
>> -if (!pte_present(pte) || !pte_user(pte) || !pte_read(pte))
>> +
>> +if (!pte_present(pte))
>> +return false;
>> +
>> +if (!(pte_read(pte) || pte_exec(pte)))
>>  return false;
>>  
>>  if (write && !pte_write(pte))
>> diff --git a/arch/powerpc/mm/book3s64/hash_utils.c 
>> b/arch/powerpc/mm/book3s64/hash_utils.c
>> index ad2afa08e62e..b2eda22195f0 100644
>> --- a/arch/powerpc/mm/book3s64/hash_utils.c
>> +++ b/arch/powerpc/mm/book3s64/hash_utils.c
>> @@ -310,9 +310,26 @@ unsigned long htab_convert_pte_flags(unsigned long 
>> pteflags, unsigned long flags
>>  else
>>

Re: [PATCH 2/3] PCI: dwc: Rename to .get_dbi_offset in struct dw_pcie_ep_ops

[Serge Semin]

On Mon, Nov 13, 2023 at 10:32:59AM +0900, Yoshihiro Shimoda wrote:
> Since meaning of .func_conf_select is difficult to understand,
> rename it to .get_dbi_offset.

This change looks good. Thanks.
Reviewed-by: Serge Semin 

There are redundant initializers will have been left after this patch
is applied, but it will be naturally fixed in the next patch.

-Serge(y)

> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  .../pci/controller/dwc/pci-layerscape-ep.c|   5 +-
>  .../pci/controller/dwc/pcie-designware-ep.c   | 108 +-
>  drivers/pci/controller/dwc/pcie-designware.h  |   2 +-
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c   |   4 +-
>  4 files changed, 59 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c 
> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> index 4e4b687ef508..961ff1b719a1 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -184,8 +184,7 @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 
> func_no,
>   }
>  }
>  
> -static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> - u8 func_no)
> +static unsigned int ls_pcie_ep_get_dbi_offset(struct dw_pcie_ep *ep, u8 
> func_no)
>  {
>   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>   struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> @@ -198,7 +197,7 @@ static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
>   .init = ls_pcie_ep_init,
>   .raise_irq = ls_pcie_ep_raise_irq,
>   .get_features = ls_pcie_ep_get_features,
> - .func_conf_select = ls_pcie_ep_func_conf_select,
> + .get_dbi_offset = ls_pcie_ep_get_dbi_offset,
>  };
>  
>  static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c 
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index ea99a97ce504..1100671db887 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -43,14 +43,14 @@ dw_pcie_ep_get_func_from_ep(struct dw_pcie_ep *ep, u8 
> func_no)
>   return NULL;
>  }
>  
> -static unsigned int dw_pcie_ep_func_select(struct dw_pcie_ep *ep, u8 func_no)
> +static unsigned int dw_pcie_ep_get_dbi_offset(struct dw_pcie_ep *ep, u8 
> func_no)
>  {
> - unsigned int func_offset = 0;
> + unsigned int dbi_offset = 0;
>  
> - if (ep->ops->func_conf_select)
> - func_offset = ep->ops->func_conf_select(ep, func_no);
> + if (ep->ops->get_dbi_offset)
> + dbi_offset = ep->ops->get_dbi_offset(ep, func_no);
>  
> - return func_offset;
> + return dbi_offset;
>  }
>  
>  static unsigned int dw_pcie_ep_get_dbi2_offset(struct dw_pcie_ep *ep, u8 
> func_no)
> @@ -59,8 +59,8 @@ static unsigned int dw_pcie_ep_get_dbi2_offset(struct 
> dw_pcie_ep *ep, u8 func_no
>  
>   if (ep->ops->get_dbi2_offset)
>   dbi2_offset = ep->ops->get_dbi2_offset(ep, func_no);
> - else if (ep->ops->func_conf_select) /* for backward compatibility */
> - dbi2_offset = ep->ops->func_conf_select(ep, func_no);
> + else if (ep->ops->get_dbi_offset) /* for backward compatibility */
> + dbi2_offset = ep->ops->get_dbi_offset(ep, func_no);
>  
>   return dbi2_offset;
>  }
> @@ -68,14 +68,14 @@ static unsigned int dw_pcie_ep_get_dbi2_offset(struct 
> dw_pcie_ep *ep, u8 func_no
>  static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
>  enum pci_barno bar, int flags)
>  {
> - unsigned int func_offset, dbi2_offset;
> + unsigned int dbi_offset, dbi2_offset;
>   struct dw_pcie_ep *ep = &pci->ep;
>   u32 reg, reg_dbi2;
>  
> - func_offset = dw_pcie_ep_func_select(ep, func_no);
> + dbi_offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
>   dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no);
>  
> - reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
> + reg = dbi_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
>   reg_dbi2 = dbi2_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
>   dw_pcie_dbi_ro_wr_en(pci);
>   dw_pcie_writel_dbi2(pci, reg_dbi2, 0x0);
> @@ -102,16 +102,16 @@ static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie_ep 
> *ep, u8 func_no,
>   u8 cap_ptr, u8 cap)
>  {
>   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> - unsigned int func_offset = 0;
> + unsigned int dbi_offset = 0;
>   u8 cap_id, next_cap_ptr;
>   u16 reg;
>  
>   if (!cap_ptr)
>   return 0;
>  
> - func_offset = dw_pcie_ep_func_select(ep, func_no);
> + dbi_offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
>  
> - reg = dw_pcie_readw_dbi(pci, func_offset + cap_ptr);
> + reg = dw_pcie_readw_dbi(pci, dbi_offset + cap_ptr);
>   cap_id = (reg & 0x00ff);
>  
>   if (cap_id > PCI_CAP_ID_MAX)
> @@ -127,13 +127,13 @@ static u8 __dw_pcie_ep_find_next_cap(struc

Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Hans Verkuil]

Hi Shengjiu,

On 10/11/2023 06:48, Shengjiu Wang wrote:
> Fixed point controls are used by the user to configure
> a fixed point value in 64bits, which Q31.32 format.
> 
> Signed-off-by: Shengjiu Wang 

This patch adds a new control type. This is something that also needs to be
tested by v4l2-compliance, and for that we need to add support for this to
one of the media test-drivers. The best place for that is the vivid driver,
since that has already a bunch of test controls for other control types.

See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.

Can you add a patch adding a fixed point test control to vivid?

Thanks!

Hans

> ---
>  .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++--
>  .../userspace-api/media/v4l/vidioc-queryctrl.rst|  9 -
>  .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
>  drivers/media/v4l2-core/v4l2-ctrls-api.c|  5 -
>  drivers/media/v4l2-core/v4l2-ctrls-core.c   |  2 ++
>  include/uapi/linux/videodev2.h  |  1 +
>  6 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst 
> b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index e8475f9fd2cf..e7e5d78dc11e 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -162,13 +162,13 @@ still cause this situation.
>  * - __s32
>- ``value``
>- New value or current value. Valid if this control is not of type
> - ``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> - not set.
> + ``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> + ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>  * - __s64
>- ``value64``
>- New value or current value. Valid if this control is of type
> - ``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> - not set.
> + ``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> + ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>  * - char *
>- ``string``
>- A pointer to a string. Valid if this control is of type
> @@ -193,8 +193,9 @@ still cause this situation.
>  * - __s64 *
>- ``p_s64``
>- A pointer to a matrix control of signed 64-bit values. Valid if
> -this control is of type ``V4L2_CTRL_TYPE_INTEGER64`` and
> -``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is set.
> +this control is of type ``V4L2_CTRL_TYPE_INTEGER64``,
> +``V4L2_CTRL_TYPE_FIXED_POINT`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
> +is set.
>  * - struct :c:type:`v4l2_area` *
>- ``p_area``
>- A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst 
> b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> index 4d38acafe8e1..f3995ec57044 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> @@ -235,7 +235,8 @@ See also the examples in :ref:`control`.
>- ``default_value``
>- The default value of a ``V4L2_CTRL_TYPE_INTEGER``, ``_INTEGER64``,
>   ``_BOOLEAN``, ``_BITMASK``, ``_MENU``, ``_INTEGER_MENU``, ``_U8``
> - or ``_U16`` control. Not valid for other types of controls.
> + ``_FIXED_POINT`` or ``_U16`` control. Not valid for other types of
> + controls.
>  
>   .. note::
>  
> @@ -549,6 +550,12 @@ See also the examples in :ref:`control`.
>- n/a
>- A struct :c:type:`v4l2_ctrl_av1_film_grain`, containing AV1 Film 
> Grain
>  parameters for stateless video decoders.
> +* - ``V4L2_CTRL_TYPE_FIXED_POINT``
> +  - any
> +  - any
> +  - any
> +  - A 64-bit integer valued control, containing parameter which is
> +Q31.32 format.
>  
>  .. raw:: latex
>  
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions 
> b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index e61152bb80d1..2faa5a2015eb 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -167,6 +167,7 @@ replace symbol V4L2_CTRL_TYPE_AV1_SEQUENCE 
> :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_AV1_FRAME :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_AV1_FILM_GRAIN :c:type:`v4l2_ctrl_type`
> +replace symbol V4L2_CTRL_TYPE_FIXED_POINT :c:type:`v4l2_ctrl_type`
>  
>  # V4L2 capability defines
>  replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c 
> b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index 002ea6588edf..e6a0fb8d6791 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/dr

Re: [PATCH 1/2] tty: hvc: Make hvc_remove() return no value

[Nicholas Piggin]

On Mon Nov 13, 2023 at 7:57 PM AEST, Uwe Kleine-König wrote:
> On Mon, Nov 13, 2023 at 07:45:27PM +1000, Nicholas Piggin wrote:
> > On Mon Nov 6, 2023 at 7:44 AM AEST, Uwe Kleine-König wrote:
> > > The function hvc_remove() returns zero unconditionally. Make it return
> > > void instead to make it obvious that the caller doesn't need to do any
> > > error handling. Accordingly drop the error handling from
> > > hvc_opal_remove().
> > >
> > > Signed-off-by: Uwe Kleine-König 
> > 
> > IIUC these are functionally no change, just tidying and removing
> > dead code?
>
> In case this isn't only a rethorical question: There is indeed no
> change in behaviour.

Thanks, it wasn't. Your changelog and code seemed to be quite clear,
I just wanted to confirm I didn't misread or misunderstand it.

Thanks,
Nick

> hvc_remove() returned always zero, so
>
>   rc = hvc_remove(hp);
>   if (rc == 0) {
>   ... some code not changing rc ...
>   }
>   ... some more code not changing rc ...
>   return rc
>
> can be simplified to
>
>   hvc_remove(hp);
>   ... some code not changing rc ...
>   ... some more code not changing rc ...
>   return 0;
>
> Best regards
> Uwe

Re: [PATCH v2 29/37] powerpc/nohash: Replace pte_user() by pte_read()

[Aneesh Kumar K.V]

Christophe Leroy  writes:

> Le 07/11/2023 à 14:34, Aneesh Kumar K.V a écrit :
>> Christophe Leroy  writes:
>> 
>>> Le 31/10/2023 à 11:15, Aneesh Kumar K.V a écrit :
 Christophe Leroy  writes:



>>
>> 
>> We are adding the pte flags check not the map addr check there. Something 
>> like this?
>
> Well, ok, but then why do we want to do that check for ioremap() and not 
> for everything else ? vmap() for instance will not perform any such 
> check. All it does is to clear the EXEC bit.
>
> As far as I can see, no other architecture does such a check, so why is 
> it needed on powerpc at all ?
>
> Regardless, comments below.
>

Looking at ioremap_prot() I am not clear whether we can really use the
flag value argument as is. For ex: x86 does 

pgprot2cachemode(__pgprot(prot_val))

I see that we use ioremap_prot() for generic_access_phys() and with
/dev/mem and __access_remote_vm() we can get called with a user pte
mapping prot flags? 

If such an prot value can be observed then the original change to clear
EXEC and mark it privileged is required?

/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
pte = pte_exprotect(pte);
pte = pte_mkprivileged(pte);


We already handle exec in pgprot_nx() and we need add back
pte_mkprivileged()? 


-aneesh

Re: [RFC PATCH] powerpc/book3s/hash: Drop _PAGE_PRIVILEGED from PAGE_NONE

[Nicholas Piggin]

On Thu Nov 2, 2023 at 11:23 PM AEST, Aneesh Kumar K.V wrote:
> There used to be a dependency on _PAGE_PRIVILEGED with pte_savedwrite.
> But that got dropped by
> commit 6a56ccbcf6c6 ("mm/autonuma: use can_change_(pte|pmd)_writable() to 
> replace savedwrite")
>
> With this change numa fault pte (pte_protnone()) gets mapped as regular user 
> pte
> with RWX cleared (no-access).

You mean "that" above change (not *this* change), right?

> This also remove pte_user() from
> book3s/64.

Nice cleanup. That was an annoying hack.

> pte_access_permitted() now checks for _PAGE_EXEC because we now support
> EXECONLY mappings.

AFAIKS pte_exec() is not required, GUP is really only for read or
write access. It should be a separate patch if you think it's needed.

>
> Signed-off-by: Aneesh Kumar K.V 
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h | 23 +---
>  arch/powerpc/mm/book3s64/hash_utils.c| 17 +++
>  2 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h 
> b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index cb77eddca54b..7c7de7b56df0 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -17,12 +17,6 @@
>  #define _PAGE_EXEC   0x1 /* execute permission */
>  #define _PAGE_WRITE  0x2 /* write access allowed */
>  #define _PAGE_READ   0x4 /* read access allowed */
> -#define _PAGE_NA _PAGE_PRIVILEGED
> -#define _PAGE_NAX_PAGE_EXEC
> -#define _PAGE_RO _PAGE_READ
> -#define _PAGE_ROX(_PAGE_READ | _PAGE_EXEC)
> -#define _PAGE_RW (_PAGE_READ | _PAGE_WRITE)
> -#define _PAGE_RWX(_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC)
>  #define _PAGE_PRIVILEGED 0x8 /* kernel access only */
>  #define _PAGE_SAO0x00010 /* Strong access order */
>  #define _PAGE_NON_IDEMPOTENT 0x00020 /* non idempotent memory */

Did you leave PAGE_NONE as _PAGE_BASE | _PAGE_PRIVILEGED below?
Shouldn't that be changed too? Then this patch is not only hash
but also radix.

Why is the hash change required? Previously PAGE_NONE relied on
privileged bit to prevent access, now you need to handle a PTE
without that bit? In that case could that be patch 1, then the
rest patch 2?

__pte_flags_need_flush() should be updated after this too,
basically revert commit 1abce0580b894.

> @@ -119,9 +113,9 @@
>  /*
>   * user access blocked by key
>   */
> -#define _PAGE_KERNEL_RW  (_PAGE_PRIVILEGED | _PAGE_RW | 
> _PAGE_DIRTY)
>  #define _PAGE_KERNEL_RO   (_PAGE_PRIVILEGED | _PAGE_READ)
>  #define _PAGE_KERNEL_ROX  (_PAGE_PRIVILEGED | _PAGE_READ | _PAGE_EXEC)
> +#define _PAGE_KERNEL_RW  (_PAGE_PRIVILEGED | _PAGE_RW | 
> _PAGE_DIRTY)
>  #define _PAGE_KERNEL_RWX (_PAGE_PRIVILEGED | _PAGE_DIRTY | _PAGE_RW | 
> _PAGE_EXEC)
>  /*
>   * _PAGE_CHG_MASK masks of bits that are to be preserved across

No need to reorder defines.

Thanks,
Nick

> @@ -523,19 +517,14 @@ static inline bool arch_pte_access_permitted(u64 pte, 
> bool write, bool execute)
>  }
>  #endif /* CONFIG_PPC_MEM_KEYS */
>  
> -static inline bool pte_user(pte_t pte)
> -{
> - return !(pte_raw(pte) & cpu_to_be64(_PAGE_PRIVILEGED));
> -}
> -
>  #define pte_access_permitted pte_access_permitted
>  static inline bool pte_access_permitted(pte_t pte, bool write)
>  {
> - /*
> -  * _PAGE_READ is needed for any access and will be
> -  * cleared for PROT_NONE
> -  */
> - if (!pte_present(pte) || !pte_user(pte) || !pte_read(pte))
> +
> + if (!pte_present(pte))
> + return false;
> +
> + if (!(pte_read(pte) || pte_exec(pte)))
>   return false;
>  
>   if (write && !pte_write(pte))
> diff --git a/arch/powerpc/mm/book3s64/hash_utils.c 
> b/arch/powerpc/mm/book3s64/hash_utils.c
> index ad2afa08e62e..b2eda22195f0 100644
> --- a/arch/powerpc/mm/book3s64/hash_utils.c
> +++ b/arch/powerpc/mm/book3s64/hash_utils.c
> @@ -310,9 +310,26 @@ unsigned long htab_convert_pte_flags(unsigned long 
> pteflags, unsigned long flags
>   else
>   rflags |= 0x3;
>   }
> + WARN_ON(!(pteflags & _PAGE_RWX));
>   } else {
>   if (pteflags & _PAGE_RWX)
>   rflags |= 0x2;
> + else {
> + /*
> +  * PAGE_NONE will get mapped to 0b110 (slb key 1 no 
> access)
> +  * We picked 0b110 instead of 0b000 so that slb key 0 
> will
> +  * get only read only access for the same rflags.
> +  */
> + if (mmu_has_feature(MMU_FTR_KERNEL_RO))
> + rflags |= (HPTE_R_PP0 | 0x2);
> + /*
> +  * rflags = HPTE_R_N
> +  * Without KERNEL_RO feature this will result in s

Re: [PATCH 1/3] PCI: dwc: Rename to .init in struct dw_pcie_ep_ops

[Serge Semin]

Hi Yoshihiro.

On Mon, Nov 13, 2023 at 10:32:58AM +0900, Yoshihiro Shimoda wrote:
> Since the name of dw_pcie_ep_ops indicates that it's for ep obviously,
> rename a member .ep_init to .init.

Thanks for the series. This change particularly looks good. But since
you are fixing the redundant prefixes anyway, could you also fix the
dw_pcie_host_ops structure too (drop host_ prefixes from the
.host_init() and .host_deinit() fields)? The change was discussed a
while ago here
https://lore.kernel.org/linux-pci/20230802104049.GB57374@thinkpad/
in the context of your long-going patchset.

It's better to be done in the framework of a separate patch released
within this series.

-Serge(y)

> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  drivers/pci/controller/dwc/pci-dra7xx.c   | 2 +-
>  drivers/pci/controller/dwc/pci-imx6.c | 2 +-
>  drivers/pci/controller/dwc/pci-keystone.c | 2 +-
>  drivers/pci/controller/dwc/pci-layerscape-ep.c| 2 +-
>  drivers/pci/controller/dwc/pcie-artpec6.c | 2 +-
>  drivers/pci/controller/dwc/pcie-designware-ep.c   | 4 ++--
>  drivers/pci/controller/dwc/pcie-designware-plat.c | 2 +-
>  drivers/pci/controller/dwc/pcie-designware.h  | 2 +-
>  drivers/pci/controller/dwc/pcie-keembay.c | 2 +-
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 2 +-
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c   | 2 +-
>  drivers/pci/controller/dwc/pcie-uniphier-ep.c | 2 +-
>  12 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c 
> b/drivers/pci/controller/dwc/pci-dra7xx.c
> index b445ffe95e3f..f9182cd6fe67 100644
> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> @@ -436,7 +436,7 @@ dra7xx_pcie_get_features(struct dw_pcie_ep *ep)
>  }
>  
>  static const struct dw_pcie_ep_ops pcie_ep_ops = {
> - .ep_init = dra7xx_pcie_ep_init,
> + .init = dra7xx_pcie_ep_init,
>   .raise_irq = dra7xx_pcie_raise_irq,
>   .get_features = dra7xx_pcie_get_features,
>  };
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c 
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 74703362aeec..737d4d90fef2 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1093,7 +1093,7 @@ imx6_pcie_ep_get_features(struct dw_pcie_ep *ep)
>  }
>  
>  static const struct dw_pcie_ep_ops pcie_ep_ops = {
> - .ep_init = imx6_pcie_ep_init,
> + .init = imx6_pcie_ep_init,
>   .raise_irq = imx6_pcie_ep_raise_irq,
>   .get_features = imx6_pcie_ep_get_features,
>  };
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c 
> b/drivers/pci/controller/dwc/pci-keystone.c
> index 0def919f89fa..655c7307eb88 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -944,7 +944,7 @@ ks_pcie_am654_get_features(struct dw_pcie_ep *ep)
>  }
>  
>  static const struct dw_pcie_ep_ops ks_pcie_am654_ep_ops = {
> - .ep_init = ks_pcie_am654_ep_init,
> + .init = ks_pcie_am654_ep_init,
>   .raise_irq = ks_pcie_am654_raise_irq,
>   .get_features = &ks_pcie_am654_get_features,
>  };
> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c 
> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> index 3d3c50ef4b6f..4e4b687ef508 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -195,7 +195,7 @@ static unsigned int ls_pcie_ep_func_conf_select(struct 
> dw_pcie_ep *ep,
>  }
>  
>  static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> - .ep_init = ls_pcie_ep_init,
> + .init = ls_pcie_ep_init,
>   .raise_irq = ls_pcie_ep_raise_irq,
>   .get_features = ls_pcie_ep_get_features,
>   .func_conf_select = ls_pcie_ep_func_conf_select,
> diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c 
> b/drivers/pci/controller/dwc/pcie-artpec6.c
> index 9b572a2b2c9a..27ff425c0698 100644
> --- a/drivers/pci/controller/dwc/pcie-artpec6.c
> +++ b/drivers/pci/controller/dwc/pcie-artpec6.c
> @@ -370,7 +370,7 @@ static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, 
> u8 func_no,
>  }
>  
>  static const struct dw_pcie_ep_ops pcie_ep_ops = {
> - .ep_init = artpec6_pcie_ep_init,
> + .init = artpec6_pcie_ep_init,
>   .raise_irq = artpec6_pcie_raise_irq,
>  };
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c 
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index f6207989fc6a..ea99a97ce504 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -794,8 +794,8 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>   list_add_tail(&ep_func->list, &ep->func_list);
>   }
>  
> - if (ep->ops->ep_init)
> - ep->ops->ep_init(ep);
> + if (ep->ops->init)
> + ep->ops->init(ep);
>  
>   ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
>

Re: Build regressions/improvements in v6.7-rc1

[Geert Uytterhoeven]

On Mon, 13 Nov 2023, Geert Uytterhoeven wrote:

Below is the list of build error/warning regressions/improvements in
v6.7-rc1[1] compared to v6.6[2].

Summarized:
 - build errors: +20/-7
 - build warnings: +24/-8

Note that there may be false regressions, as some logs are incomplete.
Still, they're build errors/warnings.

Happy fixing! ;-)

Thanks to the linux-next team for providing the build service.

[1] 
http://kisskb.ellerman.id.au/kisskb/branch/linus/head/b85ea95d086471afb4ad062012a4d73cd328fa86/
 (238 out of 239 configs)
[2] 
http://kisskb.ellerman.id.au/kisskb/branch/linus/head/ffc253263a1375a65fa6c9f62a893e9767fbebfa/
 (all 239 configs)


*** ERRORS ***

20 error regressions:
 + /kisskb/src/include/linux/compiler_types.h: error: call to 
'__compiletime_assert_654' declared with attribute error: FIELD_PREP: value too 
large for the field:  => 435:38


powerpc-gcc5/powerpc-allyesconfig
drivers/edac/versal_edac.c: In function 'mc_probe':
num_chans = FIELD_PREP(XDDR_REG_CONFIG0_NUM_CHANS_MASK, regval);


 + {standard input}: Error: displacement to undefined symbol .L100 overflows 8-bit 
field :  => 588
 + {standard input}: Error: displacement to undefined symbol .L104 overflows 8-bit 
field :  => 588
 + {standard input}: Error: displacement to undefined symbol .L105 overflows 8-bit 
field :  => 593
 + {standard input}: Error: displacement to undefined symbol .L134 overflows 8-bit 
field :  => 598
 + {standard input}: Error: displacement to undefined symbol .L72 overflows 12-bit 
field:  => 589
 + {standard input}: Error: displacement to undefined symbol .L73 overflows 8-bit 
field :  => 580
 + {standard input}: Error: displacement to undefined symbol .L75 overflows 12-bit 
field:  => 586, 589, 606
 + {standard input}: Error: displacement to undefined symbol .L76 overflows 8-bit 
field :  => 577, 580
 + {standard input}: Error: displacement to undefined symbol .L77 overflows 8-bit 
field : 582 => 607, 585
 + {standard input}: Error: displacement to undefined symbol .L78 overflows 8-bit 
field :  => 610
 + {standard input}: Error: displacement to undefined symbol .L80 overflows 8-bit 
field :  => 607, 601
 + {standard input}: Error: displacement to undefined symbol .L81 overflows 8-bit 
field : 606 => 604, 610
 + {standard input}: Error: displacement to undefined symbol .L96 overflows 12-bit 
field:  => 602
 + {standard input}: Error: displacement to undefined symbol .L97 overflows 12-bit 
field:  => 607
 + {standard input}: Error: displacement to undefined symbol .L98 overflows 12-bit 
field:  => 602
 + {standard input}: Error: invalid operands for opcode:  => 612
 + {standard input}: Error: missing operand:  => 612
 + {standard input}: Error: pcrel too far: 601, 598, 604, 577, 595, 574 => 590, 
598, 599, 577, 596, 569, 604, 610, 572, 593
 + {standard input}: Error: unknown pseudo-op: `.l':  => 609


sh4-gcc1[123]/sh-all{mod,yes}config ICE

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 0/3] PCI: dwc: Improve code readability

[Krzysztof Wilczyński]

Hi Yoshihiro!

> This patch series is based on the latest pci.git / next branch.
[...]

Thank you for following up to tidy things up!  Much appreciated.

Now, while you are looking at things, can you also take care about the 
following:

  drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to smaller 
integer type 'enum dw_pcie_device_mode' from 'const void *' 
[-Wvoid-pointer-to-enum-cast]

This requires adding structs for each data member of the of_device_id type.

Some examples from other drivers:

  - 
https://elixir.bootlin.com/linux/v6.6.1/source/drivers/pci/controller/dwc/pcie-tegra194.c#L2440
  - 
https://elixir.bootlin.com/linux/v6.6.1/source/drivers/pci/controller/dwc/pci-keystone.c#L1074

Thank you! :)

Krzysztof

Re: [PATCH 1/2] tty: hvc: Make hvc_remove() return no value

[Uwe Kleine-König]

On Mon, Nov 13, 2023 at 07:45:27PM +1000, Nicholas Piggin wrote:
> On Mon Nov 6, 2023 at 7:44 AM AEST, Uwe Kleine-König wrote:
> > The function hvc_remove() returns zero unconditionally. Make it return
> > void instead to make it obvious that the caller doesn't need to do any
> > error handling. Accordingly drop the error handling from
> > hvc_opal_remove().
> >
> > Signed-off-by: Uwe Kleine-König 
> 
> IIUC these are functionally no change, just tidying and removing
> dead code?

In case this isn't only a rethorical question: There is indeed no
change in behaviour. hvc_remove() returned always zero, so

rc = hvc_remove(hp);
if (rc == 0) {
... some code not changing rc ...
}
... some more code not changing rc ...
return rc

can be simplified to

hvc_remove(hp);
... some code not changing rc ...
... some more code not changing rc ...
return 0;

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH v9 07/15] media: v4l2: Add audio capture and output support

[Hans Verkuil]

Hi Shengjiu,

On 10/11/2023 06:48, Shengjiu Wang wrote:
> Audio signal processing has the requirement for memory to
> memory similar as Video.
> 
> This patch is to add this support in v4l2 framework, defined
> new buffer type V4L2_BUF_TYPE_AUDIO_CAPTURE and
> V4L2_BUF_TYPE_AUDIO_OUTPUT, defined new format v4l2_audio_format
> for audio case usage.
> 
> The created audio device is named "/dev/v4l-audioX".
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  .../userspace-api/media/v4l/buffer.rst|  6 ++
>  .../media/v4l/dev-audio-mem2mem.rst   | 71 +++
>  .../userspace-api/media/v4l/devices.rst   |  1 +
>  .../media/v4l/vidioc-enum-fmt.rst |  2 +
>  .../userspace-api/media/v4l/vidioc-g-fmt.rst  |  4 ++
>  .../media/videodev2.h.rst.exceptions  |  2 +
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  4 ++
>  drivers/media/v4l2-core/v4l2-dev.c| 17 +
>  drivers/media/v4l2-core/v4l2-ioctl.c  | 53 ++
>  include/media/v4l2-dev.h  |  2 +
>  include/media/v4l2-ioctl.h| 34 +
>  include/uapi/linux/videodev2.h| 17 +
>  12 files changed, 213 insertions(+)
>  create mode 100644 
> Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst

While testing this patch series I discovered that you also need to patch
drivers/media/v4l2-core/v4l2-compat-ioctl32.c so it knows about the new
audio format.

Without that 32 bit apps will fail with a 64 bit kernel.

Regards,

Hans

> 
> diff --git a/Documentation/userspace-api/media/v4l/buffer.rst 
> b/Documentation/userspace-api/media/v4l/buffer.rst
> index 52bbee81c080..a3754ca6f0d6 100644
> --- a/Documentation/userspace-api/media/v4l/buffer.rst
> +++ b/Documentation/userspace-api/media/v4l/buffer.rst
> @@ -438,6 +438,12 @@ enum v4l2_buf_type
>  * - ``V4L2_BUF_TYPE_META_OUTPUT``
>- 14
>- Buffer for metadata output, see :ref:`metadata`.
> +* - ``V4L2_BUF_TYPE_AUDIO_CAPTURE``
> +  - 15
> +  - Buffer for audio capture, see :ref:`audio`.
> +* - ``V4L2_BUF_TYPE_AUDIO_OUTPUT``
> +  - 16
> +  - Buffer for audio output, see :ref:`audio`.
>  
>  
>  .. _buffer-flags:
> diff --git a/Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst 
> b/Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst
> new file mode 100644
> index ..68faecfe3a02
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst
> @@ -0,0 +1,71 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +
> +.. _audiomem2mem:
> +
> +
> +Audio Memory-To-Memory Interface
> +
> +
> +An audio memory-to-memory device can compress, decompress, transform, or
> +otherwise convert audio data from one format into another format, in memory.
> +Such memory-to-memory devices set the ``V4L2_CAP_AUDIO_M2M`` capability.
> +Examples of memory-to-memory devices are audio codecs, audio preprocessing,
> +audio postprocessing.
> +
> +A memory-to-memory audio node supports both output (sending audio frames from
> +memory to the hardware) and capture (receiving the processed audio frames
> +from the hardware into memory) stream I/O. An application will have to
> +setup the stream I/O for both sides and finally call
> +:ref:`VIDIOC_STREAMON ` for both capture and output to
> +start the hardware.
> +
> +Memory-to-memory devices function as a shared resource: you can
> +open the audio node multiple times, each application setting up their
> +own properties that are local to the file handle, and each can use
> +it independently from the others. The driver will arbitrate access to
> +the hardware and reprogram it whenever another file handler gets access.
> +
> +Audio memory-to-memory devices are accessed through character device
> +special files named ``/dev/v4l-audio``
> +
> +Querying Capabilities
> +=
> +
> +Device nodes supporting the audio memory-to-memory interface set the
> +``V4L2_CAP_AUDIO_M2M`` flag in the ``device_caps`` field of the
> +:c:type:`v4l2_capability` structure returned by the :c:func:`VIDIOC_QUERYCAP`
> +ioctl.
> +
> +Data Format Negotiation
> +===
> +
> +The audio device uses the :ref:`format` ioctls to select the capture format.
> +The audio buffer content format is bound to that selected format. In addition
> +to the basic :ref:`format` ioctls, the :c:func:`VIDIOC_ENUM_FMT` ioctl must 
> be
> +supported as well.
> +
> +To use the :ref:`format` ioctls applications set the ``type`` field of the
> +:c:type:`v4l2_format` structure to ``V4L2_BUF_TYPE_AUDIO_CAPTURE`` or to
> +``V4L2_BUF_TYPE_AUDIO_OUTPUT``. Both drivers and applications must set the
> +remainder of the :c:type:`v4l2_format` structure to 0.
> +
> +.. c:type:: v4l2_audio_format
> +
> +.. tabularcolumns:: |p{1.4cm}|p{2.4cm}|p{13.5cm}|
> +
> +.. flat-table:: struct v4l2_audio_format
> +:header-rows:  0
> +

Re: [PATCH 2/2] tty: hvc: hvc_opal: Convert to platform remove callback returning void

[Nicholas Piggin]

On Mon Nov 6, 2023 at 7:44 AM AEST, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
>
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.

Reviewed-by: Nicholas Piggin 

>
> Signed-off-by: Uwe Kleine-König 
> ---
>  drivers/tty/hvc/hvc_opal.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index 8995b253cf90..2cdf66e395cc 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -232,7 +232,7 @@ static int hvc_opal_probe(struct platform_device *dev)
>   return 0;
>  }
>  
> -static int hvc_opal_remove(struct platform_device *dev)
> +static void hvc_opal_remove(struct platform_device *dev)
>  {
>   struct hvc_struct *hp = dev_get_drvdata(&dev->dev);
>   int termno;
> @@ -242,13 +242,11 @@ static int hvc_opal_remove(struct platform_device *dev)
>   if (hvc_opal_privs[termno] != &hvc_opal_boot_priv)
>   kfree(hvc_opal_privs[termno]);
>   hvc_opal_privs[termno] = NULL;
> -
> - return 0;
>  }
>  
>  static struct platform_driver hvc_opal_driver = {
>   .probe  = hvc_opal_probe,
> - .remove = hvc_opal_remove,
> + .remove_new = hvc_opal_remove,
>   .driver = {
>   .name   = hvc_opal_name,
>   .of_match_table = hvc_opal_match,

Re: [PATCH 1/2] tty: hvc: Make hvc_remove() return no value

[Nicholas Piggin]

On Mon Nov 6, 2023 at 7:44 AM AEST, Uwe Kleine-König wrote:
> The function hvc_remove() returns zero unconditionally. Make it return
> void instead to make it obvious that the caller doesn't need to do any
> error handling. Accordingly drop the error handling from
> hvc_opal_remove().
>
> Signed-off-by: Uwe Kleine-König 

IIUC these are functionally no change, just tidying and removing
dead code? Unless I'm mistaken, then

Reviewed-by: Nicholas Piggin 

> ---
>  drivers/tty/hvc/hvc_console.c |  3 +--
>  drivers/tty/hvc/hvc_console.h |  2 +-
>  drivers/tty/hvc/hvc_opal.c| 15 +++
>  3 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index 959fae54ca39..57f5c37125e6 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -976,7 +976,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  }
>  EXPORT_SYMBOL_GPL(hvc_alloc);
>  
> -int hvc_remove(struct hvc_struct *hp)
> +void hvc_remove(struct hvc_struct *hp)
>  {
>   unsigned long flags;
>   struct tty_struct *tty;
> @@ -1010,7 +1010,6 @@ int hvc_remove(struct hvc_struct *hp)
>   tty_vhangup(tty);
>   tty_kref_put(tty);
>   }
> - return 0;
>  }
>  EXPORT_SYMBOL_GPL(hvc_remove);
>  
> diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
> index 9668f821db01..78f7543511f1 100644
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -77,7 +77,7 @@ extern int hvc_instantiate(uint32_t vtermno, int index,
>  extern struct hvc_struct * hvc_alloc(uint32_t vtermno, int data,
>const struct hv_ops *ops, int outbuf_size);
>  /* remove a vterm from hvc tty operation (module_exit or hotplug remove) */
> -extern int hvc_remove(struct hvc_struct *hp);
> +extern void hvc_remove(struct hvc_struct *hp);
>  
>  /* data available */
>  int hvc_poll(struct hvc_struct *hp);
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index 992e199e0ea8..8995b253cf90 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -235,16 +235,15 @@ static int hvc_opal_probe(struct platform_device *dev)
>  static int hvc_opal_remove(struct platform_device *dev)
>  {
>   struct hvc_struct *hp = dev_get_drvdata(&dev->dev);
> - int rc, termno;
> + int termno;
>  
>   termno = hp->vtermno;
> - rc = hvc_remove(hp);
> - if (rc == 0) {
> - if (hvc_opal_privs[termno] != &hvc_opal_boot_priv)
> - kfree(hvc_opal_privs[termno]);
> - hvc_opal_privs[termno] = NULL;
> - }
> - return rc;
> + hvc_remove(hp);
> + if (hvc_opal_privs[termno] != &hvc_opal_boot_priv)
> + kfree(hvc_opal_privs[termno]);
> + hvc_opal_privs[termno] = NULL;
> +
> + return 0;
>  }
>  
>  static struct platform_driver hvc_opal_driver = {

Re: (subset) [PATCH v5 0/5] ppc, fbdev: Clean up fbdev mmap helper

[Thomas Zimmermann]

Am 13.11.23 um 03:45 schrieb Michael Ellerman:

On Fri, 22 Sep 2023 10:04:54 +0200, Thomas Zimmermann wrote:

Clean up and rename fb_pgprotect() to work without struct file. Then
refactor the implementation for PowerPC. This change has been discussed
at [1] in the context of refactoring fbdev's mmap code.

The first two patches update fbdev and replace fbdev's fb_pgprotect()
with pgprot_framebuffer() on all architectures. The new helper's stream-
lined interface enables more refactoring within fbdev's mmap
implementation.

[...]


Patches 3-5 applied to powerpc/fixes.

[3/5] arch/powerpc: Remove trailing whitespaces
   https://git.kernel.org/powerpc/c/322948c3198cf80e7c10d953ddad24ebd85757cd
[4/5] arch/powerpc: Remove file parameter from phys_mem_access_prot code
   https://git.kernel.org/powerpc/c/1f92a844c35e483c00bab8a7b7d39c555ee799d8
[5/5] arch/powerpc: Call internal __phys_mem_access_prot() in fbdev code
   https://git.kernel.org/powerpc/c/deebe5f607d7f72f83c41163191ad0c1c4356385


Great, thanks a lot!



cheers


--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 00/10] powerpc/pseries: New character devices for system parameters and VPD

[Michal Suchánek]

Hello,

On Thu, Oct 26, 2023 at 06:56:36PM -0500, Nathan Lynch wrote:
> Nathan Lynch via B4 Relay 
> writes:
> > I have made changes to librtas to prefer the new interfaces and
> > verified that existing clients work correctly with the new code.
> 
> Unfortunately I made a mistake in testing this time and introduced a
> boot-time oops:
> 
> BUG: Kernel NULL pointer dereference on read at 0x0018
> Faulting instruction address: 0xc004223c
> Oops: Kernel access of bad area, sig: 7 [#1]
> LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Tainted: GW  6.6.0-rc2+ #129
> NIP:  c004223c LR: c0042238 CTR: 
> REGS: c2c579d0 TRAP: 0300   Tainted: GW   (6.6.0-rc2+)
> MSR:  80001033   CR: 28000284  XER: 
> CFAR: c0042008 DAR: 0018 DSISR: 0008 IRQMASK: 3 
> GPR00: c0042238 c2c57c70 c1f5eb00  
> GPR04: c294cd08 0002 c2c579b4  
> GPR08:  0002 c2c0da80  
> GPR12:  c5e4  02097728 
> GPR16:  0001 02097b80 020975b8 
> GPR20: 020976f0 020974e8 030feb00 030feb00 
> GPR24: 2008  0001 c28f3d70 
> GPR28: 02d31020 c2cac268 c2d31020  
> NIP [c004223c] do_enter_rtas+0xcc/0x460
> LR [c0042238] do_enter_rtas+0xc8/0x460
> Call Trace:
> [c2c57c70] [c0042238] do_enter_rtas+0xc8/0x460 (unreliable)
> [c2c57cc0] [c0042e34] rtas_call+0x434/0x490
> [c2c57d20] [c00fd584] papr_sysparm_get+0xe4/0x230
> [c2c57db0] [c20267d0] pSeries_probe+0x2f0/0x5fc
> [c2c57e80] [c200a318] setup_arch+0x11c/0x524
> [c2c57f10] [c200418c] start_kernel+0xcc/0xc1c
> [c2c57fe0] [c000e788] start_here_common+0x1c/0x20
> 
> This was introduced by patch #4 "powerpc/rtas: Warn if per-function lock
> isn't held": __do_enter_rtas() is now attempting token -> descriptor
> lookups unconditionally, before the xarray for that has been initialized.
> 
> With that change reverted, the series tests OK.

What's the status here?

Can this move on with the 4th patch skipped, or is new revision
expected?

Thanks

Michal