Re: [v3 PATCH 3/3] powernv-cpufreq: Treat pstates as opaque 8-bit values
On Wed, Dec 13, 2017 at 5:57 PM, Gautham R. Shenoy wrote: > From: "Gautham R. Shenoy" > > On POWER8 and POWER9, the PMSR and the PMCR registers define pstates > to be 8-bit wide values. The device-tree exports pstates as 32-bit > wide values of which the lower byte is the actual pstate. > > The current implementation in the kernel treats pstates as integer > type, since it used to use the sign of the pstate for performing some > boundary-checks. This is no longer required after the patch > "powernv-cpufreq: Fix pstate_to_idx() to handle non-continguous > pstates". > > So, in this patch, we modify the powernv-cpufreq driver to uniformly > treat pstates as opaque 8-bit values obtained from the device-tree or > the PMCR. This simplifies the extract_pstate() helper function since > we no longer no longer require to worry about the sign-extentions. > > Signed-off-by: Gautham R. Shenoy > --- > drivers/cpufreq/powernv-cpufreq.c | 47 > --- > 1 file changed, 19 insertions(+), 28 deletions(-) > > diff --git a/drivers/cpufreq/powernv-cpufreq.c > b/drivers/cpufreq/powernv-cpufreq.c > index 8e3dbca..8a4e2ce 100644 > --- a/drivers/cpufreq/powernv-cpufreq.c > +++ b/drivers/cpufreq/powernv-cpufreq.c > @@ -110,12 +110,11 @@ struct global_pstate_info { > * hashtable > */ > struct pstate_idx_revmap_data { > - int pstate_id; > + u8 pstate_id; > unsigned int cpufreq_table_idx; > struct hlist_node hentry; > }; > > -u32 pstate_sign_prefix; > static bool rebooting, throttled, occ_reset; > > static const char * const throttle_reason[] = { > @@ -170,14 +169,9 @@ enum throttle_reason_type { > bool wof_enabled; > } powernv_pstate_info; > > -static inline int extract_pstate(u64 pmsr_val, unsigned int shift) > +static inline u8 extract_pstate(u64 pmsr_val, unsigned int shift) > { > - int ret = ((pmsr_val >> shift) & 0xFF); > - > - if (!ret) > - return ret; > - > - return (pstate_sign_prefix | ret); > + return ((pmsr_val >> shift) & 0xFF); > } So we just added this and moved from an int to u8. I was going to ask if we still need an int in patch1, but I thought the driver dealt with just integers because of the larger framework. Balbir Singh.
Re: [v3 PATCH 2/3] powernv-cpufreq: Fix pstate_to_idx() to handle non-continguous pstates
On Wed, Dec 13, 2017 at 5:57 PM, Gautham R. Shenoy wrote: > From: "Gautham R. Shenoy" > > The code in powernv-cpufreq, makes the following two assumptions which > are not guaranteed by the device-tree bindings: > > 1) Pstate ids are continguous: This is used in pstate_to_idx() to >obtain the reverse map from a pstate to it's corresponding >entry into the cpufreq frequency table. > > 2) Every Pstate should always lie between the max and the min >pstates that are explicitly reported in the device tree: This >is used to determine whether a pstate reported by the PMSR is >out of bounds. > > Both these assumptions are unwarranted and can change on future > platforms. While this is a good thing, I wonder if it is worth the complexity. Pstates are contiguous because they define transitions in incremental value of change in frequency and I can't see how this can be broken in the future? Balbir Singh.
Re: [v3 PATCH 1/3] powernv-cpufreq: Add helper to extract pstate from PMSR
On Wed, Dec 13, 2017 at 5:57 PM, Gautham R. Shenoy wrote: > From: "Gautham R. Shenoy" > > On POWERNV platform, the fields for pstates in the Power Management > Status Register (PMSR) and the Power Management Control Register > (PMCR) are 8-bits wide. On POWER8 the pstates are negatively numbered > while on POWER9 they are positively numbered. > > The device-tree exports pstates as 32-bit entries. The device-tree > implementation sign-extends the 8-bit pstate values to obtain the > corresponding 32-bit entry. > > Eg: On POWER8, a pstate value 0x82 [-126] is represented in the > device-tree as 0xfff82 while on POWER9, the same value 0x82 [130] > is represented in the device-tree as 0x0082. > > The powernv-cpufreq driver implementation represents pstates using the > integer type. In multiple places in the driver, the code interprets > the pstates extracted from the PMSR as a signed byte and assigns it to > a integer variable to get the sign-extention. > > On POWER9 platforms which have greater than 128 pstates, this results > in the driver performing incorrect sign-extention, and thereby > treating a legitimate pstate (say 130) as an invalid pstates (since it > is interpreted as -126). > > This patch fixes the issue by implementing a helper function to > extract Pstates from PMSR register, and correctly sign-extend it to be > consistent with the values provided by the device-tree. > > Signed-off-by: Gautham R. Shenoy > --- This looks better Acked-by: Balbir Singh Balbir Singh
Re: [PATCH V2] cxl: Add support for ASB_Notify on POWER9
Hi Christophe, Thank you for the patch! Yet something to improve: [auto build test ERROR on char-misc/char-misc-testing] [also build test ERROR on v4.15-rc3 next-20171215] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Christophe-Lombard/cxl-Add-support-for-ASB_Notify-on-POWER9/20171204-135557 config: powerpc-currituck_defconfig (attached as .config) compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=powerpc All errors (new ones prefixed by >>): >> arch/powerpc/kernel/process.o:(___ksymtab_gpl+clear_thread_tidr+0x0): >> undefined reference to `clear_thread_tidr' >> arch/powerpc/kernel/process.o:(___ksymtab_gpl+set_thread_tidr+0x0): >> undefined reference to `set_thread_tidr' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH v3 00/11] ASoC: fsl_ssi: Clean up - coding style level
Hello Mark, Can you please take the first two patches since Timur acked? For the rest ones, we probably still need some discussion. Thanks Nicolin On Dec 15, 2017 20:53, "Timur Tabi" wrote: On 12/13/17 5:18 PM, Nicolin Chen wrote: > Additionally, in order to fix/work-around hardware bugs and design > flaws, the driver made a lot of compromise so now its program flow > looks very complicated and it's getting hard to maintain or update. > > So I am going to clean up the driver on both coding style level and > program flow level. > I'm okay with everything except patch #3. On Dec 15, 2017 20:53, "Timur Tabi" wrote: On 12/13/17 5:18 PM, Nicolin Chen wrote: > Additionally, in order to fix/work-around hardware bugs and design > flaws, the driver made a lot of compromise so now its program flow > looks very complicated and it's getting hard to maintain or update. > > So I am going to clean up the driver on both coding style level and > program flow level. > I'm okay with everything except patch #3.
Re: powerpc64 kernel panic if you disable CONFIG_PPC_TRANSACTIONAL_MEM?
On Sun, Dec 17, 2017 at 8:34 AM, Rob Landley wrote: > I just added a ppc64 target to https://github.com/landley/mkroot which > means I built 4.14 with the attached miniconfig and ran it with the > attached qemu command line, and it works fine as is but if you remove > the transactional mem line from the config the kernel panics instead > of launching a shell prompt: > > init[1]: unhandled signal 4 at 10001a04 nip 10001a04 > lr 1002ebe8 code 1 > Kernel panic - not syncing: Attempted to kill init! exitcode=0x0004 > what does the instruction at 10001a04 look like? Balbir Singh.
powerpc64 kernel panic if you disable CONFIG_PPC_TRANSACTIONAL_MEM?
I just added a ppc64 target to https://github.com/landley/mkroot which means I built 4.14 with the attached miniconfig and ran it with the attached qemu command line, and it works fine as is but if you remove the transactional mem line from the config the kernel panics instead of launching a shell prompt: init[1]: unhandled signal 4 at 10001a04 nip 10001a04 lr 1002ebe8 code 1 Kernel panic - not syncing: Attempted to kill init! exitcode=0x0004 CPU: 0 PID: 1 Comm: init Not tainted 4.14.0 #1 Call Trace: [ce02fa40] [c04ba730] dump_stack+0xb0/0xf0 (unreliable) [ce02fa80] [c00602a0] panic+0x138/0x2f8 [ce02fb20] [c006541c] do_exit+0xa9c/0xaa0 [ce02fbe0] [c00654d8] do_group_exit+0x58/0xf0 [ce02fc20] [c0073274] get_signal+0x1c4/0x6b0 [ce02fd10] [c00142a0] do_signal+0x60/0x290 [ce02fe00] [c001461c] do_notify_resume+0x8c/0xd0 [ce02fe30] [c000b630] ret_from_except_lite+0x5c/0x60 Rebooting in 1 seconds.. Rob powerpc64le.miniconf Description: Binary data qemu-powerpc64le.sh Description: Bourne shell script
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On Sat, Dec 16, 2017 at 12:31:34PM -0600, Timur Tabi wrote: > On 12/16/17 11:49 AM, Nicolin Chen wrote: > >I never said that I don't agree with Timur. Every change here is > >to simplify things. As long as Timur or any reviewer feels one of > >new comments is harder to understand, I am totally fine to rework. > >I respect everyone's opinion, but I hope everyone can respect my > >effort too by telling me which one needs to rework and why? > > I do respect it. I apologize if I came across as unduly harsh. I believe everyone here has a good heart to make things better. So I am not and won't take it personally. Let's just act more cooperative. Thanks.
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
First of all, thanks a lot for the review. And I will send a v4 after I refine these comments. But please please don't think in the way like "you can touch it unless it's untrue." I never said anything or anyone is wrong here. As the other patches that shortens variable names, this patch just does something similar. The point here is just to make the driver necessarily explained while being brief. I am totally fine if you feel some of my new comments are worse. I will refine it until you get satisfied. And I hope you (or anyone else) can tell me more about what is wrong with my new comments instead of asking me to drop them all. I locally have more than 20 patches based on this one. Any change I make to this one will give me a lot of trouble to rebase them. So I am more willing to refine those that people really feel hard to understand. On Sat, Dec 16, 2017 at 11:15:43AM -0600, Timur Tabi wrote: > On 12/13/17 5:18 PM, Nicolin Chen wrote: > > >-/* Used when using fsl-ssi as sound-card. This is only used by ppc and > >- * should be replaced with simple-sound-card. */ > > struct platform_device *pdev; > > Is this comment no longer true? It's moved to the comments of structure fsl_ssi. Since we defined a pdev at the first place, we should have explained it along with the definition. > >-/** > >- * fsl_ssi_isr: SSI interrupt handler > >- * > >- * Although it's possible to use the interrupt handler to send and receive > >- * data to/from the SSI, we use the DMA instead. Programming is more > >- * complicated, but the performance is much better. > >- * > >- * This interrupt handler is used only to gather statistics. > >- * > >- * @irq: IRQ of the SSI device > >- * @dev_id: pointer to the fsl_ssi structure for this SSI device > >- */ > > What's wrong with this comment? Nothing wrong. An interrupt handler is way too common sense in a driver code. I am okay to retain it if you strongly feel it's that necessary. But I would feel more plausible to clean it away. > >- * Clear RX or TX FIFO to remove samples from the previous > >- * stream session which may be still present in the FIFO and > >- * may introduce bad samples and/or channel slipping. > >- * > >- * Note: The SOR is not documented in recent IMX datasheet, but > >- * is described in IMX51 reference manual at section 56.3.3.15. > >+/** > >+ * Clear remaining data in the FIFO to avoid dirty data or channel slipping > > I think the original is better, unless there's something untrue about it. It's literally stating the same thing. And SOR register comments are moved to the header file. > >- * We are running on a SoC which does not support online SSI > >- * reconfiguration, so we have to enable all necessary flags at once > >- * even if we do not use them later (capture and playback configuration) > >+ * Online configuration is not supported > >+ * Enable or Disable all necessary bits at once > > Ditto The code also does disabling as well however it doesn't mention at all. Just like you might have hard time to understand my new comments, I also had a hard time to understand this one. So I'll have to change it. My new comments are shorter but covers both enable and disable. I could make it more descriptive if necessary. > >-/* > >- * Configure single direction units while the SSI unit is running > >- * (online configuration) > >- */ > >+/* Online configure single direction while SSI is running */ > > Ditto It's literally the same but shorter. I don't think anyone would have trouble to understand mine... > >-/* > >- * Be sure the Tx FIFO is filled when TE is set. > >- * Otherwise, there are some chances to start the > >- * playback with some void samples inserted first, > >- * generating a channel slip. > >- * > >- * First, SSIEN must be set, to let the FIFO be filled. > >- * > >- * Notes: > >- * - Limit this fix to the DMA case until FIQ cases can > >- * be tested. > >- * - Limit the length of the busy loop to not lock the > >- * system too long, even if 1-2 loops are sufficient > >- * in general. > >- */ > > What's wrong with this comment? I have new comments covering necessary steps. But I could bring some parts back if that makes you happy. > >-/* > >- * Note that these below aren't just normal registers. > >- * They are a way to disable or enable bits in SACCST > >- * register: > >- * - writing a '1' bit at some position in SACCEN sets the > >- * relevant bit in SACCST, > >- * - writing a '1' bit at some position in SACCDIS unsets > >- * the relevant bit in SACCST register. > >- *
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On 12/16/17 11:49 AM, Nicolin Chen wrote: I never said that I don't agree with Timur. Every change here is to simplify things. As long as Timur or any reviewer feels one of new comments is harder to understand, I am totally fine to rework. I respect everyone's opinion, but I hope everyone can respect my effort too by telling me which one needs to rework and why? I do respect it. I apologize if I came across as unduly harsh.
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On Sat, Dec 16, 2017 at 09:30:14AM -0800, Caleb Crome wrote: > Having come to work on this driver with very little knowledge about > kernel programming, and i.MX, I have to agree with Timur. It's an > amazingly complex driver (with support of so many variants). By > eliminating verbose commentary, it's also wiping away a lot of > knowledge. The more sparse commentary makes things harder to > understand for newcomers, or really anybody who isn't already steeped > in knowledge about the SSI port and linux, and it's interaction with > DMA. I never said that I don't agree with Timur. Every change here is to simplify things. As long as Timur or any reviewer feels one of new comments is harder to understand, I am totally fine to rework. I respect everyone's opinion, but I hope everyone can respect my effort too by telling me which one needs to rework and why? Thanks Nicolin
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On 12/16/17 11:30 AM, Caleb Crome wrote: Having come to work on this driver with very little knowledge about kernel programming, and i.MX, I have to agree with Timur. It's an amazingly complex driver (with support of so many variants). By eliminating verbose commentary, it's also wiping away a lot of knowledge. The more sparse commentary makes things harder to understand for newcomers, or really anybody who isn't already steeped in knowledge about the SSI port and linux, and it's interaction with DMA. This is exactly why I wrote the comments the way I did. As I was learning about the hardware and ASoC drivers, whenever I was confused about something and then figured it out, I would add a comment about that. I figured if it wasn't obvious to me, it wouldn't be obvious to anyone else. And since this was one of the first ASoC drivers, and the first to use device tree, it would become a reference driver for years to come. That made it even more important that I document everything I learned. I am pleased that other developers kept up that commenting style. This patch destroys all of that.
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
Hi, On Sat, Dec 16, 2017 at 10:27:06AM -0600, Timur Tabi wrote: > On 12/16/17 12:10 AM, Nicolin Chen wrote: > >Hi, > > > >I am outside so can't use mutt. Sorry for that. > > > >This comment is going to be replaced in the 2nd set anyway because the > >whole function will be replaced. > > So you're asking me to review comment changes that will soon be deleted? I never said "deleted". > Can you send out a new patch without changes to comments, so that I can > focus on the ones that matter? Please don't make any assumption. I am trying my best to do that. And that's the reason why I have this patch here. I have already done as much as I can to integrate all comments here. Thanks Nicolin
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On Sat, Dec 16, 2017 at 9:15 AM, Timur Tabi wrote: > > On 12/13/17 5:18 PM, Nicolin Chen wrote: > >> - /* Used when using fsl-ssi as sound-card. This is only used by ppc >> and >> -* should be replaced with simple-sound-card. */ >> struct platform_device *pdev; > > > Is this comment no longer true? > >> + * 1) SSI in earlier SoCS has crtical bits in control registers that > > > critical > >> -/** >> - * fsl_ssi_isr: SSI interrupt handler >> - * >> - * Although it's possible to use the interrupt handler to send and receive >> - * data to/from the SSI, we use the DMA instead. Programming is more >> - * complicated, but the performance is much better. >> - * >> - * This interrupt handler is used only to gather statistics. >> - * >> - * @irq: IRQ of the SSI device >> - * @dev_id: pointer to the fsl_ssi structure for this SSI device >> - */ > > > What's wrong with this comment? > >> -/* >> - * Clear RX or TX FIFO to remove samples from the previous >> - * stream session which may be still present in the FIFO and >> - * may introduce bad samples and/or channel slipping. >> - * >> - * Note: The SOR is not documented in recent IMX datasheet, but >> - * is described in IMX51 reference manual at section 56.3.3.15. >> +/** >> + * Clear remaining data in the FIFO to avoid dirty data or channel slipping > > > I think the original is better, unless there's something untrue about it. > >> -* We are running on a SoC which does not support online SSI >> -* reconfiguration, so we have to enable all necessary flags at once >> -* even if we do not use them later (capture and playback >> configuration) >> +* Online configuration is not supported >> +* Enable or Disable all necessary bits at once > > > Ditto > >> - /* >> -* Configure single direction units while the SSI unit is running >> -* (online configuration) >> -*/ >> + /* Online configure single direction while SSI is running */ > > > Ditto > >> - /* >> -* Disabling the necessary flags for one of rx/tx while the >> -* other stream is active is a little bit more difficult. We >> -* have to disable only those flags that differ between both >> -* streams (rx XOR tx) and that are set in the stream that is >> -* disabled now. Otherwise we could alter flags of the other >> -* stream >> -*/ >> - >> - /* These assignments are simply vals without bits set in >> avals*/ >> + /* Exclude necessary bits for the opposite stream */ > > > Ditto > >> - /* >> -* Be sure the Tx FIFO is filled when TE is set. >> -* Otherwise, there are some chances to start the >> -* playback with some void samples inserted first, >> -* generating a channel slip. >> -* >> -* First, SSIEN must be set, to let the FIFO be >> filled. >> -* >> -* Notes: >> -* - Limit this fix to the DMA case until FIQ cases >> can >> -* be tested. >> -* - Limit the length of the busy loop to not lock >> the >> -* system too long, even if 1-2 loops are >> sufficient >> -* in general. >> -*/ > > > What's wrong with this comment? > >> - /* >> -* Note that these below aren't just normal registers. >> -* They are a way to disable or enable bits in SACCST >> -* register: >> -* - writing a '1' bit at some position in SACCEN sets the >> -* relevant bit in SACCST, >> -* - writing a '1' bit at some position in SACCDIS unsets >> -* the relevant bit in SACCST register. >> -* >> -* The two writes below first disable all channels slots, >> -* then enable just slots 3 & 4 ("PCM Playback Left Channel" >> -* and "PCM Playback Right Channel"). >> -*/ >> + /* Disable all channel slots */ > > > Ditto. > > >> -* Why are we setting up SACCST everytime we are starting a >> -* playback? >> -* Some CODECs (like VT1613 CODEC on UDOO board) like to >> -* (sometimes) set extra bits in their SLOTREQ requests. >> -* When a bit is set in a SLOTREQ request then SSI sets the >> -* relevant bit in SACCST automatically (it is enough if a bit was >> -* set in a SLOTREQ just once, bits in SACCST are 'sticky'). >> -* If an extra slot gets enabled that's a disaster for playback >> -* because some of normal left or right channel samples are >> -* redirected ins
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On 12/13/17 5:18 PM, Nicolin Chen wrote: - /* Used when using fsl-ssi as sound-card. This is only used by ppc and -* should be replaced with simple-sound-card. */ struct platform_device *pdev; Is this comment no longer true? + * 1) SSI in earlier SoCS has crtical bits in control registers that critical -/** - * fsl_ssi_isr: SSI interrupt handler - * - * Although it's possible to use the interrupt handler to send and receive - * data to/from the SSI, we use the DMA instead. Programming is more - * complicated, but the performance is much better. - * - * This interrupt handler is used only to gather statistics. - * - * @irq: IRQ of the SSI device - * @dev_id: pointer to the fsl_ssi structure for this SSI device - */ What's wrong with this comment? -/* - * Clear RX or TX FIFO to remove samples from the previous - * stream session which may be still present in the FIFO and - * may introduce bad samples and/or channel slipping. - * - * Note: The SOR is not documented in recent IMX datasheet, but - * is described in IMX51 reference manual at section 56.3.3.15. +/** + * Clear remaining data in the FIFO to avoid dirty data or channel slipping I think the original is better, unless there's something untrue about it. -* We are running on a SoC which does not support online SSI -* reconfiguration, so we have to enable all necessary flags at once -* even if we do not use them later (capture and playback configuration) +* Online configuration is not supported +* Enable or Disable all necessary bits at once Ditto - /* -* Configure single direction units while the SSI unit is running -* (online configuration) -*/ + /* Online configure single direction while SSI is running */ Ditto - /* -* Disabling the necessary flags for one of rx/tx while the -* other stream is active is a little bit more difficult. We -* have to disable only those flags that differ between both -* streams (rx XOR tx) and that are set in the stream that is -* disabled now. Otherwise we could alter flags of the other -* stream -*/ - - /* These assignments are simply vals without bits set in avals*/ + /* Exclude necessary bits for the opposite stream */ Ditto - /* -* Be sure the Tx FIFO is filled when TE is set. -* Otherwise, there are some chances to start the -* playback with some void samples inserted first, -* generating a channel slip. -* -* First, SSIEN must be set, to let the FIFO be filled. -* -* Notes: -* - Limit this fix to the DMA case until FIQ cases can -* be tested. -* - Limit the length of the busy loop to not lock the -* system too long, even if 1-2 loops are sufficient -* in general. -*/ What's wrong with this comment? - /* -* Note that these below aren't just normal registers. -* They are a way to disable or enable bits in SACCST -* register: -* - writing a '1' bit at some position in SACCEN sets the -* relevant bit in SACCST, -* - writing a '1' bit at some position in SACCDIS unsets -* the relevant bit in SACCST register. -* -* The two writes below first disable all channels slots, -* then enable just slots 3 & 4 ("PCM Playback Left Channel" -* and "PCM Playback Right Channel"). -*/ + /* Disable all channel slots */ Ditto. -* Why are we setting up SACCST everytime we are starting a -* playback? -* Some CODECs (like VT1613 CODEC on UDOO board) like to -* (sometimes) set extra bits in their SLOTREQ requests. -* When a bit is set in a SLOTREQ request then SSI sets the -* relevant bit in SACCST automatically (it is enough if a bit was -* set in a SLOTREQ just once, bits in SACCST are 'sticky'). -* If an extra slot gets enabled that's a disaster for playback -* because some of normal left or right channel samples are -* redirected instead to this extra slot. +* SACCST might be modified via AC Link by a CODEC if it sends +* extra bits in their SLOTREQ requests, which'll accidentally +* send valid data to slots other than normal playback slots. * -* A workaround implemented in fsl-asoc-card of setting an -* appropriate CODEC register so that slots 3 & 4 (t
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On 12/16/17 12:10 AM, Nicolin Chen wrote: Hi, I am outside so can't use mutt. Sorry for that. This comment is going to be replaced in the 2nd set anyway because the whole function will be replaced. So you're asking me to review comment changes that will soon be deleted? Can you send out a new patch without changes to comments, so that I can focus on the ones that matter? And please point out all comments that you think I need to rework. I am totally fine to do that. I don't think every single one is bad. And this patch has to go in as it also adds a lot of new comments. Ok.
[PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions
From: Markus Elfring Date: Sat, 16 Dec 2017 12:32:42 +0100 Omit an extra message for a memory allocation failure in these functions. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/ps3/ps3-lpm.c | 2 -- drivers/ps3/ps3-vuart.c | 1 - 2 files changed, 3 deletions(-) diff --git a/drivers/ps3/ps3-lpm.c b/drivers/ps3/ps3-lpm.c index e34de9a7d517..ac8d5725c9b4 100644 --- a/drivers/ps3/ps3-lpm.c +++ b/drivers/ps3/ps3-lpm.c @@ -1123,8 +1123,6 @@ int ps3_lpm_open(enum ps3_lpm_tb_type tb_type, void *tb_cache, lpm_priv->tb_cache_internal = kzalloc( lpm_priv->tb_cache_size + 127, GFP_KERNEL); if (!lpm_priv->tb_cache_internal) { - dev_err(sbd_core(), "%s:%u: alloc internal tb_cache " - "failed\n", __func__, __LINE__); result = -ENOMEM; goto fail_malloc; } diff --git a/drivers/ps3/ps3-vuart.c b/drivers/ps3/ps3-vuart.c index b7f300b79ffd..bbaad30a876f 100644 --- a/drivers/ps3/ps3-vuart.c +++ b/drivers/ps3/ps3-vuart.c @@ -929,7 +929,6 @@ static int ps3_vuart_bus_interrupt_get(void) vuart_bus_priv.bmp = kzalloc(sizeof(struct ports_bmp), GFP_KERNEL); if (!vuart_bus_priv.bmp) { - pr_debug("%s:%d: kzalloc failed.\n", __func__, __LINE__); result = -ENOMEM; goto fail_bmp_malloc; } -- 2.15.1
[PATCH 0/2] PS3: Adjustments for six function implementations
From: Markus Elfring Date: Sat, 16 Dec 2017 14:42:24 +0100 Two update suggestions were taken into account from static source code analysis. Markus Elfring (2): Delete an error message for a failed memory allocation in two functions Improve a size determination in five functions drivers/ps3/ps3-lpm.c | 2 -- drivers/ps3/ps3-sys-manager.c | 9 ++--- drivers/ps3/ps3-vuart.c | 12 +++- 3 files changed, 5 insertions(+), 18 deletions(-) -- 2.15.1
[PATCH 2/2] ps3: Improve a size determination in five functions
From: Markus Elfring Date: Sat, 16 Dec 2017 14:21:04 +0100 Replace the specification of data structures by variable references as the parameter for the operator "sizeof" to make the corresponding size determination a bit safer according to the Linux coding style convention. Signed-off-by: Markus Elfring --- drivers/ps3/ps3-sys-manager.c | 9 ++--- drivers/ps3/ps3-vuart.c | 11 +++ 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/drivers/ps3/ps3-sys-manager.c b/drivers/ps3/ps3-sys-manager.c index 73e496a72113..e7d8ef93576a 100644 --- a/drivers/ps3/ps3-sys-manager.c +++ b/drivers/ps3/ps3-sys-manager.c @@ -249,9 +249,7 @@ static int ps3_sys_manager_write(struct ps3_system_bus_device *dev, BUG_ON(header->payload_size != 8 && header->payload_size != 16); BUG_ON(header->service_id > 8); - result = ps3_vuart_write(dev, header, - sizeof(struct ps3_sys_manager_header)); - + result = ps3_vuart_write(dev, header, sizeof(*header)); if (!result) result = ps3_vuart_write(dev, payload, header->payload_size); @@ -537,11 +535,8 @@ static int ps3_sys_manager_handle_cmd(struct ps3_system_bus_device *dev) static int ps3_sys_manager_handle_msg(struct ps3_system_bus_device *dev) { - int result; struct ps3_sys_manager_header header; - - result = ps3_vuart_read(dev, &header, - sizeof(struct ps3_sys_manager_header)); + int result = ps3_vuart_read(dev, &header, sizeof(header)); if (result) return result; diff --git a/drivers/ps3/ps3-vuart.c b/drivers/ps3/ps3-vuart.c index bbaad30a876f..c82362cbaf4a 100644 --- a/drivers/ps3/ps3-vuart.c +++ b/drivers/ps3/ps3-vuart.c @@ -522,8 +522,7 @@ int ps3_vuart_write(struct ps3_system_bus_device *dev, const void *buf, } else spin_unlock_irqrestore(&priv->tx_list.lock, flags); - lb = kmalloc(sizeof(struct list_buffer) + bytes, GFP_KERNEL); - + lb = kmalloc(sizeof(*lb) + bytes, GFP_KERNEL); if (!lb) return -ENOMEM; @@ -575,9 +574,7 @@ static int ps3_vuart_queue_rx_bytes(struct ps3_system_bus_device *dev, /* Add some extra space for recently arrived data. */ bytes += 128; - - lb = kmalloc(sizeof(struct list_buffer) + bytes, GFP_ATOMIC); - + lb = kmalloc(sizeof(*lb) + bytes, GFP_ATOMIC); if (!lb) return -ENOMEM; @@ -925,9 +922,7 @@ static int ps3_vuart_bus_interrupt_get(void) return 0; BUG_ON(vuart_bus_priv.bmp); - - vuart_bus_priv.bmp = kzalloc(sizeof(struct ports_bmp), GFP_KERNEL); - + vuart_bus_priv.bmp = kzalloc(sizeof(*vuart_bus_priv.bmp), GFP_KERNEL); if (!vuart_bus_priv.bmp) { result = -ENOMEM; goto fail_bmp_malloc; -- 2.15.1
[PATCH 38/45] arch/powerpc: remove duplicate includes
These duplicate includes have been found with scripts/checkincludes.pl but they have been removed manually to avoid removing false positives. Signed-off-by: Pravin Shedge --- arch/powerpc/kernel/time.c | 2 -- arch/powerpc/lib/code-patching.c | 1 - arch/powerpc/mm/numa.c | 1 - arch/powerpc/platforms/powernv/npu-dma.c | 1 - arch/powerpc/platforms/powernv/opal.c| 1 - 5 files changed, 6 deletions(-) diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index fe6f3a2..64d0b80 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -57,7 +57,6 @@ #include #include #include -#include #include #include #include @@ -78,7 +77,6 @@ /* powerpc clocksource/clockevent code */ -#include #include static u64 rtc_read(struct clocksource *); diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index d469224..5f7e5ed 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -15,7 +15,6 @@ #include #include #include -#include #include #include diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c index adb6364f..fa4b3af 100644 --- a/arch/powerpc/mm/numa.c +++ b/arch/powerpc/mm/numa.c @@ -33,7 +33,6 @@ #include #include #include -#include #include #include #include diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c index f6cbc1a..e35b4fc 100644 --- a/arch/powerpc/platforms/powernv/npu-dma.c +++ b/arch/powerpc/platforms/powernv/npu-dma.c @@ -26,7 +26,6 @@ #include #include #include -#include #include "powernv.h" #include "pci.h" diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c index 041ddbd..e00c1c6 100644 --- a/arch/powerpc/platforms/powernv/opal.c +++ b/arch/powerpc/platforms/powernv/opal.c @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include -- 2.7.4